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

Yasumasa Suenaga yasuenag at gmail.com
Thu Jul 4 12:30:26 UTC 2019


Hi Chris,

Thank you for your review.

On 2019/07/04 8:07, Chris Plummer wrote:
> Hi Yasumasa,
> 
> Overall these changes look good, but I think there is a bit of cleanup 
> still needed.
> 
> You've changed the indentation of the help to always have a few spaces 
> before the /t. If you are going to do this, then perhaps you don't need 
> the /t anymore.

Ok, I will replace '\t' to whitespace.


> There are 3 checks for "remote != null". Shouldn't they be "remote != 
> NO_REMOTE"?

I will fix them.


> The old jstack help output is a bit more informative than your output:
> 
>      jstack [-m] [-l] [server_id@]<remote server IP or hostname>
>          (to connect to a remote debug server)
> 
> vs
> 
>      --connect <[id@]server> To operate on the remote debug server.
> 
> More specifically, you replaced <remote server IP or hostname> with 
> "server". I think the former is a bit more self explanatory.

Exactly, but it is too long to show on the console.
So I used short word: server.

If we can ignore long line, or can add newline (\n) on help message,
I can change it. What do you think?


> Also, why say "to operate on" rather than "to connect to"? The later seems more 
> direct and correct.

I will replace it to "to connect to".


> I think you should also mention sadebugd in the jhsdb help output for 
> the -connect option, and not just say "debug server". Just change it to 
> "debug server (sadebugd)".

I will add "(sadebugd)".


> There are also online jhsdb doc updates that we need to make sure are 
> eventually taken care of properly.
> 
> https://docs.oracle.com/en/java/javase/11/tools/jhsdb.html
> 
> The CSR should trigger the doc update, but you need to make sure the CSR 
> contains all the needed details and instructions for the doc writer. For 
> example, here's what the jstack docs look like:
> 
> https://docs.oracle.com/javase/7/docs/technotes/tools/share/jstack.html
> 
> *jstack*  [ option ] [server-id@]remote-hostname-or-IP
> ...
> remote-hostname-or-IP
>      remote debug server's (see jsadebugd) hostname or IP address.
> 
> It includes a reference to jsadebugd (and an actual URL link). This was 
> important for me just in doing this review, because I had to go and 
> figure out what "debug server" was all about, having never dealt with it 
> before. I found it by looking at the old jstack docs and help output.  
> Adding this to the jhsdb docs should be called out in the CSR.

I'm not an employee of Oracle, so I think I cannot change documents(s) 
which are provided by Oracle web sites.
What should I do?

Also CSR of this issue has been approved (JDK-8224979).
Can we change it?


Thanks,

Yasumasa


> thanks,
> 
> Chris
> 
> On 7/1/19 3:26 PM, Yasumasa Suenaga wrote:
>> 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