8223814: SA: jhsdb common help needs to be more detailed

[Osamu Sakamoto]

Hi David,

Thank you for reviewing.

I fixed the patch to include your comment.
Yasumasa uploaded new webrev.
<http://cr.openjdk.java.net/~ysuenaga/JDK-8223814/webrev.01/>

Thanks,
Osamu



On 5/28/19 16:39, David Holmes wrote:
> Hi Osamu,
> 
> On 28/05/2019 3:57 pm, Osamu Sakamoto wrote:
>> Hi,
>>
>> David,
>> Thank you for reviewing.
>>
>> I agree with your comment.
>> I fixed the patch and Yasumasa uploaded the webrev for this fixed patch.
>> <http://cr.openjdk.java.net/~ysuenaga/JDK-8223814/webrev.00/>
>>
>> And I attached fixed --help outputs to this mail(help2.txt).
> 
> That all looks good to me - thanks. Just one minor correction in one of 
> my suggestions:
> 
> +        System.out.println("    --core <corefile>\tTo operate on a 
> given core file.");
> 
> s/a/the/
> 
> and one minor correction to yours:
> 
> +        System.out.println("    --dumpfile <name>\tA Name of the dump 
> file.");
> 
> s/A Name/The name/
> 
> Thanks,
> David
> -----
> 
>>
>> Serugui,
>>
>> In this patch, I added angle brackets only to 
>> parameters(--pid/--core/--exe/debugd --severid/jmap --dumpfile), not 
>> descriptions of each options.
>> Does this form match your comment in this RFE?
>> <https://bugs.openjdk.java.net/browse/JDK-8223814>
>>
>>
>> Thanks,
>> Osamu
>>
>>
>> On 5/28/19 08:50, David Holmes wrote:
>>> Hi Osamu,
>>>
>>> I have a lot of "suggestions" here. Writing good help output is not 
>>> easy, and the more you try to do things the more you expose existing 
>>> problems - which is the case here I'm afraid. I didn't fully realize 
>>> the constraints these commands had in regards to how they operate 
>>> either on a live process or else using a core file and executable 
>>> together, so some of my previous guidance was a little mis-guided. 
>>> Sorry about that.
>>>
>>> On 23/05/2019 7:34 pm, Osamu Sakamoto wrote:
>>>> Hi all,
>>>>
>>>> I've made the patch that was discussed here.
>>>> <https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-May/028042.html> 
>>>>
>>>>
>>>> This patch fixes the following JBS ticket.
>>>> <https://bugs.openjdk.java.net/browse/JDK-8223814>
>>>>
>>>> I attached the patch to this email.
>>>> This patch passes serviceability/sa jtreg tests.
>>>
>>> +        System.out.println("");
>>> +        System.out.println("    --pid and --exe are mutually 
>>> execlusive, and --core only goes with --exe.");
>>> +        System.out.println("    Arguments following the --exe and 
>>> --core can be absolute or relative path.");
>>>
>>> This  partially captures the intent that the flags are mutually 
>>> exclusive but its more complex than this. I would suggest:
>>>
>>> ---
>>> The --core and --exe options must be set together to give the core 
>>> file, and associated executable, to operate on. Otherwise the --pid 
>>> option can be set to operate on a live process.
>>> The arguments for --exe and --core can use absolute or relative paths.
>>> ---
>>>
>>> +        System.out.println("    Examples: jhsdb " + mode + " --pid 
>>> <pid>");
>>> +        System.out.println("          or  jhsdb " + mode + " --exe 
>>> <executable> --core <core>");
>>>
>>> As these are examples I would substitute actual values eg:
>>>
>>> --pid 1234
>>> --core ./core.1234 --exe ./myexe
>>>
>>> ---
>>>
>>> The suggestion to use angle brackets has been taken too far - angle 
>>> brackets delimit an argument to a flag, they do not delimit a 
>>> description of the option. For example in:
>>>
>>> $jhsdb jstack --help
>>>      --locks     <to print java.util.concurrent locks>
>>>      --mixed     <to print both java and native frames (mixed mode)>
>>>      --exe       <executable image name>
>>>      --core      <path to coredump>
>>>      --pid       <pid of process to attach>
>>>
>>> The:
>>>
>>>      --locks     <to print java.util.concurrent locks>
>>>
>>> should just be:
>>>
>>>      --locks     To print java.util.concurrent locks.
>>>
>>> The same for --mixed.
>>>
>>> I suggest turning the descriptions into sentences as shown with 
>>> initial capitals and final period.
>>>
>>> But that highlights an inconsistent approach with regards to 
>>> --exe/--pid/--core as we are not describing the meaning of those 
>>> flags on the same line. For consistency we should have in this order:
>>>
>>>      --pid <pid>        To attach to and operate on the given live 
>>> process.
>>>      --core <corefile>  To operate on a given core file.
>>>      --exe <executable for corefile>
>>>
>>> This puts the focus on --core where it belongs - the --exe is just 
>>> something that has to accompany --core. So that putting it all 
>>> together we would have:
>>>
>>> $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 a given core file.
>>>     --exe <executable for corefile>
>>>
>>>     The --core and --exe options must be set together to give the core
>>>     file, and associated executable, to operate on. Otherwise the --pid
>>>     option can be set to operate on a live process.
>>>     The arguments for --exe and --core can use absolute or relative 
>>> paths.
>>> ----
>>>
>>> For:
>>>
>>>      --serverid  <unique id for this debug server>
>>>
>>> I suggest
>>>
>>>      --serverid  <id>  A unique identifier for this debug server.
>>>
>>>
>>> Thanks,
>>> David
>>> --------
>>>
>>>
>>>> Could you help? I would like to contribute it. I need a sponsor.
>>>> (My company has signed to OCA (NTT Comware Corporation))
>>>>
>>>>
>>>> Thanks,
>>>> Osamu
>>
>>