RFR(S): JDK-8059038 Create new launcher for SA tools

[Dmitry Samersoff]

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
>>
>>
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.