RFR(S): JDK-8059038 Create new launcher for SA tools
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jul 17 10:46:50 UTC 2015
Dmitry,
Thanks for the diff, it helps!
hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
Uninitialized local definition:
105 char ch;
Unneded second initialization at 111:
104 String carg = _argv[_optind];
111 carg = _argv[_optind];
It is not clear why carg can't be empty:
61 // _argv[_optind] can't be empty, so it's safe to expect at least one character
62 if (_argv[_optind].charAt(0) == '-') {
...
113 // carg can't be empty so it's safe to expect at least one character
114 if (carg.charAt(0) != '-' || carg.equals("--")) {
The _argv comes from outside of the method.
How can we be sure that the value _argv[_optind] is not empty String?
Does it comes from an assumption that the outside processing works correctly?
Would it be better to always check that it is not empty?
It feels like this code is not clear and more complex than has to be.
But I can't tell yet what has to be simplified.
For example, I do not like this part:
37 private boolean _optreset; // special handling of first call
44 _optreset = true;
108 if (_optreset) {
138 _optreset = false;
Would it be better to separate this first step from the next() method
and make it a separate method that is called reset() or init()?
Also, there is no clue why all this is necessary.
Other files look good to me.
Do you have another reviewer?
Thanks,
Serguei
On 7/17/15 2:46 AM, Dmitry Samersoff wrote:
> Serguei,
>
> Previous webrev is:
> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05.old
>
> Latest webrev is:
> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
>
> Diff between webrev.05.old and webrev.05 attached
>
> -Dmitry
>
> On 2015-07-17 01:00, serguei.spitsyn at oracle.com wrote:
>> Hi Dmitry,
>>
>> I do not see any changes.
>> Could you please, generate .06 version ?
>> In such a case, it will be much easier to compare the code.
>>
>> Thanks,
>> Serguei
>>
>> On 7/16/15 8:23 AM, Dmitry Samersoff wrote:
>>> Serguei,
>>>
>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
>>>
>>> Webrev updated in-place (press shift-reload).
>>>
>>> Code changes at ll.119.
>>>
>>> Added more comments to other places.
>>>
>>> -Dmitry
>>>
>>> On 2015-06-27 03:15, serguei.spitsyn at oracle.com wrote:
>>>> Hi Dmitry,
>>>>
>>>> Thank you for the update!
>>>> The SALauncher.java changes are really nice.
>>>> I have just couple of small comments.
>>>>
>>>> agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
>>>>
>>>> 343 // Run tmtools
>>>> 344 if (args[0].equals("jstack")) {
>>>> 345 runJSTACK(oldArgs);
>>>>
>>>> Why the comment says "Run tmtools", not jstack?
>>>> BTW, other fragments have no such a comment which is Ok at it is
>>>> obvious.
>>>>
>>>>
>>>> agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>>>
>>>> There are no checks of the carg length in several places where it is
>>>> needed:
>>>>
>>>> 61 if (_argv[_optind].charAt(0) == '-') {
>>>>
>>>> 112 if (carg.charAt(0) != '-' || carg.equals("--")) {
>>>> 117 if (carg.charAt(0) == '-' && carg.charAt(1) == '-') {
>>>> 124 carg = carg.substring(2);
>>>>
>>>> 136 ch = carg.charAt(_optopt);
>>>> 139 ch = carg.charAt(_optopt);
>>>>
>>>>
>>>> Otherwise, the fix looks good.
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 6/24/15 5:37 AM, Dmitry Samersoff wrote:
>>>>> Serguei,
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>> New webrev is here:
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05/
>>>>>
>>>>> I didn't change naming convention in SAGetoptTest.java and keep a_opt,
>>>>> b_opt etc as it gives better readability. Other concerns are addressed.
>>>>>
>>>>> BasicLauncherTest changed to use LingeredApp from testlib.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>>
>>>>> On 2015-06-24 08:32, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Some quick minor comments.
>>>>>>
>>>>>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>>>>>
>>>>>> Formatting is wrong:
>>>>>>
>>>>>> 57 if (_optind >_argv.length) {
>>>>>>
>>>>>> 71 String[] ca = carg.split("=",2);
>>>>>>
>>>>>> 80 if (los.contains(ca[0]+"=")) {
>>>>>>
>>>>>>
>>>>>> Need to use camel case for java method names:
>>>>>>
>>>>>> 55 private void extract_optarg(String opt) {
>>>>>>
>>>>>> 69 private String process_long_options(String carg, String[]
>>>>>> longOptStr) {
>>>>>>
>>>>>>
>>>>>> Need to use quotes for '-':
>>>>>>
>>>>>> 109 // End of option batch like -abc reached, expect
>>>>>> option to start from -
>>>>>>
>>>>>> Example is:
>>>>>>
>>>>>> 133 // At this point carg[0] contains '-'
>>>>>>
>>>>>>
>>>>>> Wrong indent at 87, 139, 120-121:
>>>>>>
>>>>>> 85 else {
>>>>>> 86 // Mixed style options --file name
>>>>>> 87 extract_optarg(ca[0]);
>>>>>> 88 }
>>>>>>
>>>>>> 138 else {
>>>>>> 139 ch = carg.charAt(_optopt);
>>>>>> 140 }
>>>>>>
>>>>>> 119 if (longOptStr == null || longOptStr.length ==
>>>>>> 0) {
>>>>>> 120 // No long options specified, stop options
>>>>>> processing
>>>>>> 121 return null;
>>>>>> 122 }
>>>>>>
>>>>>>
>>>>>>
>>>>>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
>>>>>>
>>>>>> Uninitialized local:
>>>>>>
>>>>>> 128 String s;
>>>>>>
>>>>>> Need to use camel case:
>>>>>>
>>>>>> 126 String exe_or_pid = null;
>>>>>>
>>>>>>
>>>>>> The main method is too long, I'd suggest to split with the
>>>>>> sub-methods for:
>>>>>> clhsdb, hsdb, jstack, jmap, jinfo
>>>>>>
>>>>>>
>>>>>> jdk_webrev/test/sun/tools/jhsdb/BasicLauncherTest.java
>>>>>>
>>>>>> Wrong indent at 82, 85:
>>>>>>
>>>>>> 80 return toolProcess.exitValue();
>>>>>> 81 } finally {
>>>>>> 82 theApp.stopApp();
>>>>>> 83 }
>>>>>> 84 } catch (IOException | InterruptedException ex) {
>>>>>> 85 throw new RuntimeException("Test ERROR " + ex,
>>>>>> ex);
>>>>>> 86 }
>>>>>>
>>>>>>
>>>>>> I do not understand what is the need for nested try statements,
>>>>>> just one
>>>>>> try would be enough:
>>>>>>
>>>>>> 54 System.out.println("Starting LingeredApp");
>>>>>> 55 try {
>>>>>> 56 try {
>>>>>> . . .
>>>>>> 81 } finally {
>>>>>> 82 theApp.stopApp();
>>>>>> 83 }
>>>>>> 84 } catch (IOException | InterruptedException ex) {
>>>>>> 85 throw new RuntimeException("Test ERROR " + ex,
>>>>>> ex);
>>>>>> 86 }
>>>>>>
>>>>>> 98 try {
>>>>>> 99 try {
>>>>>> . . .
>>>>>> 116 } finally {
>>>>>> 117 theApp.stopApp();
>>>>>> 118 }
>>>>>> 119 } catch (Exception ex) {
>>>>>> 120 throw new RuntimeException("Test ERROR " + ex, ex);
>>>>>> 121 }
>>>>>>
>>>>>>
>>>>>> Why do you catch exceptions and throw the RuntimeException's in the
>>>>>> launch() methods
>>>>>> but catch the IOException in main? Would it be better to catch any
>>>>>> Exception?
>>>>>>
>>>>>> Too many empty lines:
>>>>>>
>>>>>> 88
>>>>>> 89
>>>>>> 90
>>>>>>
>>>>>>
>>>>>> jdk_webrev/test/sun/tools/jhsdb/LingeredApp.java
>>>>>>
>>>>>> Too many empty lines:
>>>>>>
>>>>>> 275
>>>>>> 276
>>>>>>
>>>>>> 369
>>>>>>
>>>>>>
>>>>>> jdk_webrev/test/sun/tools/jhsdb/SAGetoptTest.java
>>>>>>
>>>>>> Need to use Java naming convention:
>>>>>>
>>>>>> 36 private static boolean a_opt;
>>>>>> 37 private static boolean b_opt;
>>>>>> 38 private static boolean c_opt;
>>>>>> 39 private static boolean e_opt;
>>>>>> 40 private static boolean mixed_opt;
>>>>>> 41
>>>>>> 42 private static String d_value;
>>>>>> 43 private static String exe_value;
>>>>>> 44 private static String core_value;
>>>>>>
>>>>>> Wrong indent 2 instead of 4:
>>>>>>
>>>>>> 70 if (s.equals("a")) {
>>>>>> 71 a_opt = true;
>>>>>> 72 continue;
>>>>>> 73 }
>>>>>> 74
>>>>>> 75 if (s.equals("b")) {
>>>>>> 76 b_opt = true;
>>>>>> 77 continue;
>>>>>> 78 }
>>>>>> 79
>>>>>> 80 if (s.equals("c")) {
>>>>>> 81 c_opt = true;
>>>>>> 82 continue;
>>>>>> 83 }
>>>>>> 84
>>>>>> 85 if (s.equals("e")) {
>>>>>> 86 e_opt = true;
>>>>>> 87 continue;
>>>>>> 88 }
>>>>>> 89
>>>>>> 90 if (s.equals("mixed")) {
>>>>>> 91 mixed_opt = true;
>>>>>> 92 continue;
>>>>>> 93 }
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 6/23/15 7:06 AM, Dmitry Samersoff wrote:
>>>>>>> Hi Everybody,
>>>>>>>
>>>>>>> Please review the changes:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.04/
>>>>>>>
>>>>>>> I'm about to file CCC request for it but would like to get internal
>>>>>>> feedback before that.
>>>>>>>
>>>>>>> This fix is introducing native launcher jhsdb for serviceability
>>>>>>> agent.
>>>>>>>
>>>>>>> jhsdb
>>>>>>>
>>>>>>> will launch command line debugger clhsdb
>>>>>>>
>>>>>>> jhsdb jstack file core
>>>>>>> jhsdb jmap file core
>>>>>>> jhsdb jinfo file core
>>>>>>>
>>>>>>> will launch corresponding SA based utility.
>>>>>>>
>>>>>>>
>>>>>>> -Dmitry
>>>>>>>
>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150717/ea61e248/attachment.html>
More information about the serviceability-dev
mailing list