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
