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

David Holmes david.holmes at oracle.com
Thu Jul 4 21:07:13 UTC 2019


Hi Chris,

On 5/07/2019 5:24 am, Chris Plummer wrote:
> Hi Yasumasa,
> 
> On 7/4/19 5:30 AM, Yasumasa Suenaga wrote:
>> 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?
> I don't see why not. Isn't there already help output that relies on 
> newlines?
>>
>>
>>> 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?
> Since the CSR drives the documentation update, I was just asking that 
> the CSR be updated to include all information needed to properly update 
> the docs.

I'm not aware of any automatic doc updating process driven by the CSR. 
If there is a doc task to be done then a specific task needs to be 
created for it. In this case it is the manpage for jhsdb that needs to 
be updated, which is now a dev task.

David

>>
>> Also CSR of this issue has been approved (JDK-8224979).
>> Can we change it?
>>
> I'm not sure. I think that's a question for Joe Darcy. You would also 
> need to update it for the suggested help output changes above.
> 
> thanks,
> 
> Chris
>>
>> 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