RFR(S): JDK-8059038 Create new launcher for SA tools
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Jun 27 00:15:31 UTC 2015
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/20150626/8a46904a/attachment.html>
More information about the serviceability-dev
mailing list