Hi Yasumasa,
Sorry, just noticed one other thing:
<--core --exe> and <--pid> and <--connect> are mutually exclusive.
I'm not sure why you are using angled brackets here. It's not done in
other parts of the help output that reference options by name. Also, no
need to mention --exe since there is an earlier comment in the help
saying in must be used together with --core. This should help to
simplify the help message. I would suggest:
--core, --pid, and --connect are mutually exclusive.
There's also a typo in the version of the help that does not include
--connect:
76 System.out.println(" <--core --exe> and <--pid> are mutually
eexclusive.");
Notice "eexclusive".
Otherwise it looks good.
thanks,
Chris
On 7/8/19 5:06 AM, Yasumasa Suenaga wrote:
Hi Chris,
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8209790/webrev.06/
It shows help messages 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 (debugd).
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
(debugd).
<--core --exe> and <--pid> and <--connect> are mutually exclusive.
Examples: jhsdb jstack --pid 1234
or jhsdb jstack --core ./core.1234 --exe ./myexe
or jhsdb jstack --connect debugserver
or jhsdb jstack --connect id@debugserver
```
Thanks,
Yasumasa
On 2019/07/08 16:14, Chris Plummer wrote:
Sorry, I meant "jhsdb debugd", not "clhsdb debugd".
Chris
On 7/8/19 12:09 AM, Chris Plummer wrote:
Hi Yasumasa,
I just noticed one more thing:
65 System.out.println(" --connect <[id@]host> To
connect to a remote debug server (sadebugd).");
Earlier on I incorrectly said that jstack and other tools had a
references to "sadebugd" in the help output. It is actually
"jsadebugd". But that was the name of the command line tool, which
is now gone. Instead now we have "clhsdb debugd ...", so maybe it
should just mention "debugd".
thanks,
Chris
On 7/7/19 10:24 PM, Chris Plummer wrote:
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@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
|
