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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Jul 9 18:46:20 UTC 2019


Hi Yasmasa,

I'll look at it today.

Thanks,
Serguei


On 7/8/19 4:16 PM, Yasumasa Suenaga wrote:
> 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