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.
zombie:hs-rt#diff -ur webrevs_001/hotspot_webrev/raw_files
webrevs/hotspot_webrev/raw_files
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-16 17:54:08.000000000 +0300
@@ -58,6 +58,7 @@
throw new RuntimeException("Not enough arguments for '" + opt +
"'");
}
+ // _argv[_optind] can't be empty, so it's safe to expect at least one
character
if (_argv[_optind].charAt(0) == '-') {
throw new RuntimeException("Argument is expected for '" + opt +
"'");
}
@@ -109,18 +110,21 @@
// End of option batch like '-abc' reached, expect option to start
from '-'
carg = _argv[_optind];
+ // carg can't be empty so it's safe to expect at least one
character
if (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);
}
@@ -133,9 +137,11 @@
// At this point carg[0] contains '-'
_optreset = false;
_optopt = 1;
+ // Length of carg checked below
ch = carg.charAt(_optopt);
}
else {
+ // Length of carg checked below
ch = carg.charAt(_optopt);
}
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-16 17:54:09.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;
