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

[Chris Plummer]

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