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;