RFR(S): JDK-8059038 Create new launcher for SA tools
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jul 17 13:28:38 UTC 2015
On 7/17/15 6:21 AM, Dmitry Samersoff wrote:
> Serguei,
>
> new webrev:
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
The webrev is the same.
I do not see the changes you claim below.
Could you, please, generate a webrev with another version number?
Thanks,
Serguei
>
> diff to webrev.05.old attached
>
> please see also below.
>
> On 2015-07-17 13:46, serguei.spitsyn at oracle.com wrote:
>> Dmitry,
>>
>> Thanks for the diff, it helps!
>>
>> hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
>>
>> Uninitialized local definition:
>>
>> 105 char ch;
> changed. ch is initialized later, so we actually don't need it.
>
>
>> Unneded second initialization at 111:
>> 104 String carg = _argv[_optind];
>> 111 carg = _argv[_optind];
> fixed.
>
>> 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("--")) {
> changed.
>
> An array passed to getopt is result of splitting arguments string, so no
> empty array element possible. But changed it to be on safe side.
>
>> 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()?
> Reset called every time when we finish the option batch:
>
> prog -xzvf filename /*reset here*/ -abc
>
>> Also, there is no clue why all this is necessary.
> This is a port of standard BSD getopt (based on C++ code I wrote back in
> 2004), that takes care of all possible option combinations and allow to
> process it uniform way.
>
> I would love to have it JDK-wide instead of a separate parser for each tool.
>
>> Other files look good to me.
>> Do you have another reviewer?
> Stefan Larsen reviewed one of the previous versions.
>
> -Dmitry
>
>>
>>
>> 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
>>>>>>>>>
>>>>>>>>>
>
More information about the serviceability-dev
mailing list