RFR: 8209790: SA tools not providing option to connect to debug server

[Jean Christophe Beyler]

Hi Yasumasa,

I'm not an official reviewer but I don't see an issue with the CSR (except
that this seems to be bringing a fork in the tools with some handling
remote and others not).

However, this code is really repetitive and this is not the place to do a
big refactor probably but we could do a few nits perhaps:
   - Instead of every tool calling commonHelp with an additional flag you
could divide into commonHelp and commonHelpWithRemote for the tools and
they both call the current commonHelp with that boolean; so that when we
are looking at the tool code we know what we are getting... So something
like:

private static boolean commonHelp(String mode, boolean canConnectToRemote) {
    ..
 }

private static boolean commonHelp(String mode) {
   return commonHelp(mode, false);
}

private static boolean commonHelpWithRemote(String mode) {
   return commonHelp(mode, false);
}

and that way the tools that change are just going from:
-        return commonHelp("jmap");
+        return commonHelpWithRemote("jmap");

- In the same vein, instead of passing null to the buildAttachArgs; you
could  make a variable null:

-        buildAttachArgs(newArgs, pid, exe, core, true);
+        String noRemote = null;
+        buildAttachArgs(newArgs, pid, exe, core, noRemote, true);


-
http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdUtils.java.html
   Nit: you have empty lines at l64 and l73

-
http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/test/hotspot/jtreg/serviceability/sa/sadebugd/DebugdConnectTest.java.html
    Nit : you have an empty line at l110

   - In runTests; if DebugdUtils implemented Closeable, you could just do a
try-with-resources instead of the finally clause...

All of these are details, I just thought I'd mention them :)
Jc

On Tue, Jun 4, 2019 at 6:44 PM Yasumasa Suenaga <yasuenag at gmail.com> wrote:

> PING: Could you review them?
>
> >    JBS: https://bugs.openjdk.java.net/browse/JDK-8209790
> >    CSR: https://bugs.openjdk.java.net/browse/JDK-8224979
> >    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
>
> CSR status is provisional. So I need reviewers both CSR and webrev.
>
>
> Thanks,
>
> Yasumasa
>
>
> 2019年5月29日(水) 22:37 Yasumasa Suenaga <yasuenag at gmail.com>:
> >
> > Hi all,
> >
> > Please review this change:
> >
> >    JBS: https://bugs.openjdk.java.net/browse/JDK-8209790
> >    CSR: https://bugs.openjdk.java.net/browse/JDK-8224979
> >    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.00/
> >
> > In JDK 8 or earlier, some tools (jstack, jmap, jinfo) can connect to
> > debug server (jsadebugd). However it has not done so since JDK 9 because
> > jhsdb cannot accept the attach request to debug server.
> > So I want to introduce new option `--remote` to connect to debug server.
> >
> > I created CSR for this issue. So please review it together.
> >
> >
> > Thanks,
> >
> > Yasumasa
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190604/d2885aa2/attachment.html>