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

Chris Plummer chris.plummer at oracle.com
Thu Jul 4 19:24:17 UTC 2019


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.
>
> 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