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.
