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

[serguei.spitsyn at oracle.com]

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/20150623/b8e1fd65/attachment-0001.html>