RFR(S): JDK-8059038 Create new launcher for SA tools
Dmitry Samersoff
dmitry.samersoff at oracle.com
Fri Jul 17 14:13:48 UTC 2015
Serguei,
Sorry for typeo
new webrev:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.06
-Dmitry
On 2015-07-17 16:28, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>>>>>>
>>>>>>>>>>
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list