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