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

[Yasumasa Suenaga]

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/

It is P3 bug, but I want to fix it before JDK 13 RDP 1 if possible.


Thanks,

Yasumasa


2019年6月5日(水) 14:06 Yasumasa Suenaga <yasuenag at gmail.com>:
>
> Hi Jc,
>
> Thank you for your comment!
> I updated a webrev:
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
>
> >    - In runTests; if DebugdUtils implemented Closeable, you could just do a try-with-resources instead of the finally clause...
>
> I created DebugdUtils for convenience class for attach - detach
> mechanism of debug server.
> IMHO it is prefer "detach" to "close" in this case.
>
>
> Thanks,
>
> Yasumasa
>
>
> 2019年6月5日(水) 11:34 Jean Christophe Beyler <jcbeyler at google.com>:
> >
> > 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