Sorry, new webrev is here: http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.01/
Yasumasa 2019年6月10日(月) 11:27 Yasumasa Suenaga <yasue...@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 <yasue...@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 <jcbey...@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 <yasue...@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 <yasue...@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
