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

Yasumasa Suenaga yasuenag at gmail.com
Mon Jul 1 22:26:21 UTC 2019


PING: Could you review it?

>    http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/


Yasumasa


On 2019/06/25 16:47, Yasumasa Suenaga wrote:
> Hi,
> 
> This enhancement has been retargeted to 14, and the CSR has been approved.
> I uploaded a webrev for 14. Could you review it?
> 
>    http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.03/
> 
> It includes the fix to rename `--remote` to `--connect` that is
> suggested in CSR.
> It passed tests on submit repo.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> 2019年6月17日(月) 22:11 Yasumasa Suenaga <yasuenag at gmail.com>:
>>
>> Hi David,
>>
>> On 2019/06/17 21:42, David Holmes wrote:
>>> Hi Yasumasa,
>>>
>>> On 17/06/2019 6:50 pm, Yasumasa Suenaga wrote:
>>>> Hi David,
>>>>
>>>> 8209790 is filed as a bug.
>>>
>>> I don't agree this is a "bug" - sorry. For this to be a bug there must
>>> be some specification of behaviour that the implementation is violating.
>>> Is that the case? To me this is missing functionality which makes it an
>>> enhancement.
>>
>> The feature for connecting to remote debug server has been provided JDK
>> 8 or earlier. However it was missed since JDK 9. So I think we can
>> handle it as a "bug".
>> Anyway, I added jdk13-enhancement-request label to JBS. I'm waiting for
>> the approval.
>>
>>
>>>> According to [1], I think we can push the fix to jdk/jdk13. Does it
>>>> correct?
>>>>
>>>> I'm not sure which version (13 or 14) should be set on JBS before
>>>> pushing.
>>>
>>> Just to clarify the process here, you don't want to set the "Fix
>>> Version" to 13 or 14 in JBS before pushing. That will be set based on
>>> the repo you push to. If you push to jdk/jdk it will be 14. If you push
>>> to jdk/jdk13 it will be 13. Any fix pushed to jdk/jdk13 will be
>>> "automatically" forward-ported to jdk/jdk and thus 14.
>>
>> Thanks! I got it.
>>
>>
>> Yasumasa
>>
>>
>>> David
>>> -----
>>>
>>>> (Of course I will push it after the CSR is approved.)
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> [1] http://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003051.html
>>>>
>>>>
>>>> On 2019/06/17 17:06, David Holmes wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> On 17/06/2019 8:52 am, Yasumasa Suenaga wrote:
>>>>>> 2019年6月17日(月) 6:47 serguei.spitsyn at oracle.com
>>>>>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
>>>>>> <mailto:serguei.spitsyn at 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?
>>>>>
>>>>> JDK 13 has now entered RDP1 so if you want this to go into 13 it will
>>>>> need special approval:
>>>>>
>>>>> http://openjdk.java.net/jeps/3#Late-Enhancement-Request-Process
>>>>>
>>>>> Otherwise push to jdk/jdk and it will be for JDK 14.
>>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>
>>>>>>      Thanks,
>>>>>>      Serguei
>>>>>>
>>>>>>
>>>>>>      On 6/16/19 14:44, serguei.spitsyn at oracle.com
>>>>>>      <mailto:serguei.spitsyn at 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.spitsyn at oracle.com
>>>>>>      <mailto:serguei.spitsyn at 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.spitsyn at oracle.com
>>>>>>      <mailto:serguei.spitsyn at 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<yasuenag at gmail.com
>>>>>>      <mailto: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
>>>>>>      <mailto: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 <mailto: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
>>>>>> <mailto: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 <mailto: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
>>>>>>       >>>>>
>>>>>>       >>>
>>>>>>       >
>>>>>>


More information about the serviceability-dev mailing list