Serguei,
new webrev:
http://cr.openjdk.java.net/~dsamersoff/JDK-8059038/webrev.05
diff to webrev.05.old attached
please see also below.
On 2015-07-17 13:46, serguei.spit...@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.spit...@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.spit...@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.spit...@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.
diff -ur
webrevs_001/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
---
webrevs_001/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
2015-06-24 15:14:58.000000000 +0300
+++
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SAGetopt.java
2015-07-17 16:01:36.000000000 +0300
@@ -58,7 +58,7 @@
throw new RuntimeException("Not enough arguments for '" + opt +
"'");
}
- if (_argv[_optind].charAt(0) == '-') {
+ if (! _argv[_optind].isEmpty() && _argv[_optind].charAt(0) == '-') {
throw new RuntimeException("Argument is expected for '" + opt +
"'");
}
@@ -95,32 +95,32 @@
public String next(String optStr, String[] longOptStr) {
- if (_optind >= _argv.length) {
+ if (_optind >= _argv.length || _argv[_optind] == null) {
// All arguments processed
return null;
}
String carg = _argv[_optind];
- char ch;
+ char ch = '-';
_optarg = null;
if (_optreset) {
-
// End of option batch like '-abc' reached, expect option to start
from '-'
- carg = _argv[_optind];
- if (carg.charAt(0) != '-' || carg.equals("--")) {
+ if (carg.isEmpty() || carg.charAt(0) != '-' || carg.equals("--")) {
// Stop processing on -- or first non-option argument;
return null;
}
- if (carg.charAt(0) == '-' && carg.charAt(1) == '-') {
+ if (carg.startsWith("--")) {
// Handle long options, it can't be combined so it's simple
if (longOptStr == null || longOptStr.length == 0) {
- // No long options specified, stop options processing
+ // No long options expected, stop options processing
return null;
}
++ _optind;
+
+ // at this point carg contains at least one character besides
--
carg = carg.substring(2);
return processLongOptions(carg, longOptStr);
}
diff -ur
webrevs_001/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
---
webrevs_001/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
2015-06-24 15:15:01.000000000 +0300
+++
webrevs/hotspot_webrev/raw_files/new/agent/src/share/classes/sun/jvm/hotspot/SALauncher.java
2015-07-17 16:01:38.000000000 +0300
@@ -329,7 +329,7 @@
String[] oldArgs = Arrays.copyOfRange(args, 1, args.length);
- // Run SA
+ // Run SA interactive mode
if (args[0].equals("clhsdb")) {
runCLHSDB(oldArgs);
return;
@@ -340,7 +340,7 @@
return;
}
- // Run tmtools
+ // Run SA tmtools mode
if (args[0].equals("jstack")) {
runJSTACK(oldArgs);
return;
