2019年6月17日(月) 6:47 serguei.spit...@oracle.com <serguei.spit...@oracle.com>:
> Forgot to tell... > This can be pushed only after the CSR is approved. > Sure! And I will push it when I get second reviewer. BTW should I push this change to jdk/jdk? or push to jdk/jdk13 manually? Thanks, Yasumasa > Thanks, > Serguei > > > On 6/16/19 14:44, serguei.spit...@oracle.com wrote: > > Hi Yasumasa, > > > > > > On 6/16/19 07:22, Yasumasa Suenaga wrote: > >> Hi Serguei, > >> > >> >>> One minor suggestion is to use the final field NO_REMOTE instead > >> >>> of null for initialization of the local variable "remote". > >> > >> I fixed it on new webrev. Could you check again? > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.02/ > > > > > > It looks good. > > Thanks you for the update! > > > >> > >> >> IMHO refactoring should be worked on another issue. > >> > > >> > Agreed. > >> > >> I filed it to JBS: > >> https://bugs.openjdk.java.net/browse/JDK-8226204 > > > > Thank you for filing the enhancement! > > > > Thanks. > > Serguei > > > > > >> Thanks, > >> > >> Yasumasa > >> > >> > >> On 2019/06/15 15:10, serguei.spit...@oracle.com wrote: > >>> Hi Yasumasa, > >>> > >>> > >>> On 6/14/19 21:11, Yasumasa Suenaga wrote: > >>>> Hi Serguei, > >>>> > >>>> Thank you for your comment! > >>>> > >>>> On 2019/06/15 8:00, serguei.spit...@oracle.com wrote: > >>>>> Hi Yasumasa, > >>>>> > >>>>> I've added myself as a reviewer, so you can finalize it now. > >>>> > >>>> I moved CSR to Finalized, and added a comment for your question. > >>> > >>> Okay, thanks! > >>> > >>> > >>>>> The fix looks pretty good to me. > >>>>> > >>>>> One minor suggestion is to use the final field NO_REMOTE instead > >>>>> of null for initialization of the local variable "remote". > >>>> > >>>> I will fix that. > >>>> > >>>> > >>>>> Also just an observation that there is some room for option > >>>>> processing refactoring. > >>>> > >>>> Your suggestion handles all options in one parser method. > >>>> I concern it might be complex for option validation. > >>>> (e.g. `jmap -heap` is allowed, but `jstack -heap` is not allowed.) > >>> > >>> This concern is not valid as the list allowed options allowed for each > >>> jhsdb sub-command is controlled with the longOpts argument. > >>> > >>> jmap has: > >>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>> "heap", "binaryheap", "dumpfile=", "histo", > >>> "clstats", "finalizerinfo"}; > >>> > >>> but jstack has: > >>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>> "mixed", "locks"}; > >>> > >>>> IMHO refactoring should be worked on another issue. > >>> > >>> Agreed. > >>> > >>> > >>>> If you are OK in above, I will upload new webrev. > >>> > >>> Yes, I'm Okay with it. > >>> > >>> > >>> Thanks, > >>> Serguei > >>> > >>> > >>>> Thanks, > >>>> > >>>> Yasumasa > >>>> > >>>> > >>>>> All the jhsdb sub-commands do execute similar loops like this: > >>>>> while((s = sg.next(null, longOpts)) != null) { > >>>>> . . . > >>>>> } > >>>>> > >>>>> It can be moved into a separate method instead. > >>>>> The longOpts can passed in arguments. > >>>>> > >>>>> It can be something like this: > >>>>> > >>>>> private ArrayList<String> processOptions(final String[] oldArgs, > >>>>> final String[] > >>>>> longOpts, > >>>>> boolean allowEmpty) { > >>>>> SAGetopt sg = new SAGetopt(oldArgs); > >>>>> ArrayList<String> newArgs = new ArrayList(); > >>>>> > >>>>> String pid = null; > >>>>> String exe = null; > >>>>> String core = null; > >>>>> String s = null; > >>>>> String dumpfile = null; > >>>>> String remote = NO_REMOTE; > >>>>> boolean requestHeapdump = false; > >>>>> > >>>>> while((s = sg.next(null, longOpts)) != null) { > >>>>> if (s.equals("exe")) { > >>>>> exe = sg.getOptarg(); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("core")) { > >>>>> core = sg.getOptarg(); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("pid")) { > >>>>> pid = sg.getOptarg(); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("remote")) { > >>>>> remote = sg.getOptarg(); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("mixed")) { > >>>>> newArgs.add("-m"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("locks")) { > >>>>> newArgs.add("-l"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("heap")) { > >>>>> newArgs.add("-heap"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("binaryheap")) { > >>>>> requestHeapdump = true; > >>>>> continue; > >>>>> } > >>>>> if (s.equals("dumpfile")) { > >>>>> dumpfile = sg.getOptarg(); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("histo")) { > >>>>> newArgs.add("-histo"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("clstats")) { > >>>>> newArgs.add("-clstats"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("finalizerinfo")) { > >>>>> newArgs.add("-finalizerinfo"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("flags")) { > >>>>> newArgs.add("-flags"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("sysprops")) { > >>>>> newArgs.add("-sysprops"); > >>>>> continue; > >>>>> } > >>>>> if (s.equals("serverid")) { > >>>>> String serverid = sg.getOptarg(); > >>>>> if (serverid != null) { > >>>>> newArgs.add(serverid); > >>>>> } > >>>>> continue; > >>>>> } > >>>>> } > >>>>> > >>>>> if (!requestHeapdump && (dumpfile != null)) { > >>>>> throw new IllegalArgumentException("Unexpected > >>>>> argument dumpfile"); > >>>>> } > >>>>> if (requestHeapdump) { > >>>>> if (dumpfile == null) { > >>>>> newArgs.add("-heap:format=b"); > >>>>> } else { > >>>>> newArgs.add("-heap:format=b,file=" + dumpfile); > >>>>> } > >>>>> } > >>>>> buildAttachArgs(newArgs, pid, exe, core, remote, > >>>>> allowEmpty); > >>>>> > >>>>> return newArgs; > >>>>> } > >>>>> > >>>>> private static void runCLHSDB(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid="}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, true); > >>>>> > >>>>> CLHSDB.main(newArgs.toArray(new String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runHSDB(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid="}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, true); > >>>>> > >>>>> HSDB.main(newArgs.toArray(new String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runJSTACK(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>>>> "mixed", "locks"}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, false); > >>>>> > >>>>> JStack jstack = new JStack(false, false); > >>>>> jstack.runWithArgs(newArgs.toArray(new > >>>>> String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runJMAP(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>>>> "heap", "binaryheap", "dumpfile=", "histo", > >>>>> "clstats", "finalizerinfo"}; > >>>>> > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, false); > >>>>> > >>>>> JMap.main(newArgs.toArray(new String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runJINFO(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>>>> "flags", "sysprops"}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, false); > >>>>> > >>>>> JInfo.main(newArgs.toArray(new String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runJSNAP(String[] oldArgs) { > >>>>> String[] longOpts = {"exe=", "core=", "pid=", "remote=", > >>>>> "all"}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, false); > >>>>> JSnap.main(newArgs.toArray(new String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> private static void runDEBUGD(String[] oldArgs) { > >>>>> // By default SA agent classes prefer Windows process > >>>>> debugger > >>>>> // to windbg debugger. SA expects special properties to > >>>>> be set > >>>>> // to choose other debuggers. We will set those here before > >>>>> // attaching to SA agent. > >>>>> System.setProperty("sun.jvm.hotspot.debugger.useWindbgDebugger", > >>>>> "true"); > >>>>> > >>>>> String[] longOpts = {"exe=", "core=", "pid=", "serverid="}; > >>>>> ArrayList<String> newArgs = processOptions(oldArgs, > >>>>> longOpts, false); > >>>>> > >>>>> // delegate to the actual SA debug server. > >>>>> sun.jvm.hotspot.DebugServer.main(newArgs.toArray(new > >>>>> String[newArgs.size()])); > >>>>> } > >>>>> > >>>>> > >>>>> Please, let me know what do you think. > >>>>> > >>>>> Thanks, > >>>>> Serguei > >>>>> > >>>>> > >>>>> On 6/9/19 7:29 PM, Yasumasa Suenaga wrote: > >>>>>> 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 > >>>>> > >>> > > > >
