Hi Osamu,
This update looks good to me.
Thanks,
Serguei
On 5/28/19 17:37, 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
