[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8223814: SA: jhsdb common help needs to be more detailed
From: Osamu Sakamoto <sakamoto.osamu () nttcom ! co ! jp>
Date: 2019-05-29 4:43:35
Message-ID: bb69db7a-8284-2900-63a9-e25db007e3f6 () nttcom ! co ! jp
[Download RAW message or body]
Hi David,
Thank you for reviewing.
I will request Yasumasa to push the webrev to jdk/jdk .
Thanks,
Osamu
On 5/29/19 13:24, David Holmes wrote:
> 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
>>>>
>>>>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic