RFR(S): JDK-8059038 Create new launcher for SA tools
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Jul 17 14:29:38 UTC 2015
Dmitry,
Thanks for new webrev!
A couple of comments on
hotspot_webrev/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
The following fragment has the same invariant for both branches:
136 ch = carg.charAt(_optopt);
137 }
138 else {
139 ch = carg.charAt(_optopt);
140 }
It can be replaced with:
}
ch = carg.charAt(_optopt);
One more suggestion would be to refactor the if (_optreset) { ... } with a cal to a new method optReset().
But I leave it up to you.
Thumbs up from me.
Thanks,
Serguei
On 7/17/15 7:13 AM, Dmitry Samersoff wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150717/7026f15a/attachment-0001.html>
More information about the serviceability-dev
mailing list