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