RFR(S): JDK-8059038 Create new launcher for SA tools
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jul 16 22:00:33 UTC 2015
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.spitsyn at 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.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
>>>>>
>>>>>
>
More information about the serviceability-dev
mailing list