PING: RFR: 8209790: SA tools not providing option to connect to debug server
Yasumasa Suenaga
yasuenag at gmail.com
Mon Jul 8 23:16:02 UTC 2019
Thank you Chris!
Serguei, do you agree with this change?
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.07/
If you are ok, I will reopen the CSR (JDK-8224979).
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028624.html
Thanks,
Yasumasa
On 2019/07/09 8:08, Chris Plummer wrote:
> Looks good! Sorry about the high number of iterations. I should have caught some things sooner.
>
> thanks,
>
> Chris
>
> On 7/8/19 3:43 PM, Yasumasa Suenaga wrote:
>> Hi Chris,
>>
>> Thank you for your comment.
>> I fixed it in new webrev:
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.07/
>>
>> Could you check again?
>>
>>
>> Yasumasa
>>
>>
>> On 2019/07/09 4:11, Chris Plummer wrote:
>>> Hi Yasumasa,
>>>
>>> Sorry, just noticed one other thing:
>>>
>>>> <--core --exe> and <--pid> and <--connect> are mutually exclusive.
>>> I'm not sure why you are using angled brackets here. It's not done in other parts of the help output that reference options by name. Also, no need to mention --exe since there is an earlier comment in the help saying in must be used together with --core. This should help to simplify the help message. I would suggest:
>>>
>>> --core, --pid, and --connect are mutually exclusive.
>>>
>>> There's also a typo in the version of the help that does not include --connect:
>>>
>>> 76 System.out.println(" <--core --exe> and <--pid> are mutually eexclusive.");
>>>
>>> Notice "eexclusive".
>>>
>>> Otherwise it looks good.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>> On 7/8/19 5:06 AM, Yasumasa Suenaga wrote:
>>>> Hi Chris,
>>>>
>>>> I uploaded new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.06/
>>>>
>>>> It shows help messages as below:
>>>>
>>>> ```
>>>> $ jhsdb jstack --help
>>>> --locks To print java.util.concurrent locks.
>>>> --mixed To print both Java and native frames (mixed mode).
>>>> --pid <pid> To attach to and operate on the given live process.
>>>> --core <corefile> To operate on the given core file.
>>>> --exe <executable for corefile>
>>>> --connect [<id>@]<host> To connect to a remote debug server (debugd).
>>>>
>>>> The --core and --exe options must be set together to give the core
>>>> file, and associated executable, to operate on. They can use
>>>> absolute or relative paths.
>>>> The --pid option can be set to operate on a live process.
>>>> The --connect option can be set to connect to a debug server (debugd).
>>>> <--core --exe> and <--pid> and <--connect> are mutually exclusive.
>>>>
>>>> Examples: jhsdb jstack --pid 1234
>>>> or jhsdb jstack --core ./core.1234 --exe ./myexe
>>>> or jhsdb jstack --connect debugserver
>>>> or jhsdb jstack --connect id at debugserver
>>>> ```
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/07/08 16:14, Chris Plummer wrote:
>>>>> Sorry, I meant "jhsdb debugd", not "clhsdb debugd".
>>>>>
>>>>> Chris
>>>>>
>>>>> On 7/8/19 12:09 AM, Chris Plummer wrote:
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> I just noticed one more thing:
>>>>>>
>>>>>> 65 System.out.println(" --connect <[id@]host> To connect to a remote debug server (sadebugd).");
>>>>>>
>>>>>> Earlier on I incorrectly said that jstack and other tools had a references to "sadebugd" in the help output. It is actually "jsadebugd". But that was the name of the command line tool, which is now gone. Instead now we have "clhsdb debugd ...", so maybe it should just mention "debugd".
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 7/7/19 10:24 PM, Chris Plummer wrote:
>>>>>>> Hi Yasumasa,
>>>>>>>
>>>>>>> I think it should be "[<id>@]<host>". Angle brackets are used to indicate non-literal values (ones to be replaced by something the user chooses), but the '@' is a literal value, so should not be enclosed in the angle brackets.
>>>>>>>
>>>>>>> Also one minor thing. You ended up duplicating the //:
>>>>>>>
>>>>>>> 60 // // --connect <[id@]host>
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 7/6/19 5:55 PM, Yasumasa Suenaga wrote:
>>>>>>>> Hi Chris,
>>>>>>>>
>>>>>>>> Thank you for your advise.
>>>>>>>> I uploaded new webrev. How about this?
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.04/
>>>>>>>>
>>>>>>>> I replaced "server" to "host" as you said.
>>>>>>>> Other comments from you also have been fixed in it.
>>>>>>>>
>>>>>>>> For example, `jhsdb jstack --help` shows as below:
>>>>>>>>
>>>>>>>> ```
>>>>>>>> $ jhsdb jstack --help
>>>>>>>> --locks To print java.util.concurrent locks.
>>>>>>>> --mixed To print both Java and native frames (mixed mode).
>>>>>>>> --pid <pid> To attach to and operate on the given live process.
>>>>>>>> --core <corefile> To operate on the given core file.
>>>>>>>> --exe <executable for corefile>
>>>>>>>> --connect <[id@]host> To connect to a remote debug server (sadebugd).
>>>>>>>>
>>>>>>>> The --core and --exe options must be set together to give the core
>>>>>>>> file, and associated executable, to operate on. They can use
>>>>>>>> absolute or relative paths.
>>>>>>>> The --pid option can be set to operate on a live process.
>>>>>>>> The --connect option can be set to connect to a debug server (sadebugd).
>>>>>>>> <--core --exe> and <--pid> and <--connect> are exclusive.
>>>>>>>>
>>>>>>>> Examples: jhsdb jstack --pid 1234
>>>>>>>> or jhsdb jstack --core ./core.1234 --exe ./myexe
>>>>>>>> or jhsdb jstack --connect debugserver
>>>>>>>> or jhsdb jstack --connect id at debugserver
>>>>>>>> ```
>>>>>>>>
>>>>>>>> If you are ok, I will reopen CSR (JDK-8224979).
>>>>>>>>
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2019/07/06 6:26, Chris Plummer wrote:
>>>>>>>>> On 7/4/19 3:53 PM, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi Chris,
>>>>>>>>>>
>>>>>>>>>> On 2019/07/05 4:24, 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?
>>>>>>>>>>
>>>>>>>>>> As you can see on the CSR (JDK-8224979), format of jhsdb help messages is:
>>>>>>>>>>
>>>>>>>>>> <option name> <blank (includes tab)> <description>
>>>>>>>>>>
>>>>>>>>>> Thus I think long option / description is not comfortable.
>>>>>>>>>> If we need to describe more, I think we should write it as below:
>>>>>>>>>>
>>>>>>>>>> --connect <[server-id@]remote server IP or hostname>
>>>>>>>>>> To connect to a remote debug server.
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>> We already have multiline examples like:
>>>>>>>>>
>>>>>>>>> |--pid <pid> To attach to and operate on the given live process.
>>>>>>>>>
>>>>>>>>> Is your concerned that the long option name will extend into the columns where the description normally is? If so, you can start the description on a new line as you suggest, or maybe come up with a different name for the option that isn't quite as long. My main objection to just saying "server" is it in no way conveys we are talking about an IP host. Even just saying <host> would be better. It is also more correct since the host isn't necessarily what someone would consider to be a server (it could just be someone's desktop). <server-id> on the other hand is the correct term because we are talking about the id of the debug server running on a particular host.
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>> |
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
More information about the serviceability-dev
mailing list