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

[Yasumasa Suenaga]

Sorry, new webrev is here:
  http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/

Yasumasa

2019年6月10日(月) 11:27 Yasumasa Suenaga <yasuenag at gmail.com>:
>
> 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