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

[David Holmes]

Ship it! :)

Thanks,
David

On 29/05/2019 10:37 am, Osamu Sakamoto wrote:
> 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
>>>
>>>