-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Friday, March 1, 2019 12:22 PM
To: Yasumasa Suenaga <yasue...@gmail.com>; 臧琳 <zangl...@jd.com>
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
<serviceability-dev@openjdk.java.net>
Subject: Re: Protocol version of Attach API
On 1/03/2019 1:54 pm, Yasumasa Suenaga wrote:
Hi,
I agree with David. I think we should backout 8215622.
Note that I conceded that if Lin's proposal for changing the 4 back
to 3 fixed
everything, then I'm okay with making that fix rather than a full
backout re-
do.
I'm not at all convinced we need change the number of args at the
protocol
level, regardless of the number of apparent "args" the command has.
Cheers,
David
We should re-do 8215622 completely for all platforms (includes
Windows) and support receiving requests from earlier serviceability
tools.
Problems about arguments of AttachListener should be worked as another
issues.
Thanks,
Yasumasa
2019年3月1日(金) 12:39 臧琳 <zangl...@jd.com>:
Hi David,
My understanding about those "arg" is that they are trying to
make
limitation of the overall message length and make it convince to
communicate
with sockets.
I will do more test try to see whether changing back to 3 makes
everything fine, and then prepare a webrev.
Thanks.
Lin
________________________________________
From: David Holmes <david.hol...@oracle.com>
Sent: Thursday, February 28, 2019 7:55:01 PM
To: 臧琳; Yasumasa Suenaga
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
Subject: Re: Protocol version of Attach API
Hi Lin,
On 28/02/2019 7:30 pm, 臧琳 wrote:
Hi David,
I am a little confused, do you think it is proper to made
the patch as a
fix of https://bugs.openjdk.java.net/browse/JDK-8219721 so that we
don't
need to backout and REDO?
Generally I'd prefer to do the backout and then apply the revised fix
as it will make the changes easier to track.
However, if you are saying that everything works fine just by
changing the 4 back to 3 everywhere, then that does seem a very
simple fix to apply directly.
I admit that if that does work then I really don't understand what
these "arg" values actually means. :( Though it would explain why
windows appears to work fine even though it was left at 3.
Thanks,
David
Thanks,
LIn
________________________________________
From: 臧琳
Sent: Thursday, February 28, 2019 4:50:12 PM
To: David Holmes; Yasumasa Suenaga
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
Subject: Re: Protocol version of Attach API
Dear All,
I have tried simply recover the max argument count makes
jmap -
histo works and also makes the compatibility works fine.
Following are the changes I made:
diff -r 07dd34f487d4 src/hotspot/share/services/attachListener.hpp
--- a/src/hotspot/share/services/attachListener.hpp Thu Feb 28
02:47:39 2019 +0100
+++ b/src/hotspot/share/services/attachListener.hpp Thu Feb 28
16:48:19 2019 +0800
@@ -106,7 +106,7 @@
enum {
name_length_max = 16, // maximum length of name
arg_length_max = 1024, // maximum length of argument
- arg_count_max = 4 // maximum number of arguments
+ arg_count_max = 3 // maximum number of arguments
};
// name of special operation that can be enqueued when all diff
-r 07dd34f487d4
src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 02:47:39 2019 +0100
+++
b/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
Thu
Feb 28 16:48:19 2019 +0800
@@ -138,7 +138,7 @@
* Execute the given command in the target VM.
*/
InputStream execute(String cmd, Object ... args) throws
AgentLoadException, IOException {
- assert args.length <= 4; // includes null
+ assert args.length <= 3; // includes null
// did we detach?
synchronized (this) {
@@ -166,7 +166,7 @@
writeString(s, PROTOCOL_VERSION);
writeString(s, cmd);
- for (int i = 0; i < 4; i++) {
+ for (int i = 0; i < 3; i++) {
if (i < args.length && args[i] != null) {
writeString(s, (String)args[i]);
} else {
diff -r 07dd34f487d4
src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 02:47:39 2019 +0100
+++
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 16:48:19 2019 +0800
@@ -143,7 +143,7 @@
* Execute the given command in the target VM.
*/
InputStream execute(String cmd, Object ... args) throws
AgentLoadException, IOException {
- assert args.length <= 4; // includes null
+ assert args.length <= 3; // includes null
// did we detach?
synchronized (this) {
diff -r 07dd34f487d4
src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 02:47:39 2019 +0100
+++
b/src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 16:48:19 2019 +0800
@@ -139,7 +139,7 @@
* Execute the given command in the target VM.
*/
InputStream execute(String cmd, Object ... args) throws
AgentLoadException, IOException {
- assert args.length <= 4; // includes null
+ assert args.length <= 3; // includes null
// did we detach?
synchronized (this) {
diff -r 07dd34f487d4
src/jdk.attach/solaris/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/solaris/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 02:47:39 2019 +0100
+++
b/src/jdk.attach/solaris/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 16:48:19 2019 +0800
@@ -126,7 +126,7 @@
* Execute the given command in the target VM.
*/
InputStream execute(String cmd, Object ... args) throws
AgentLoadException, IOException {
- assert args.length <= 4; // includes null
+ assert args.length <= 3; // includes null
// first check that we are still attached
int door;
diff -r 07dd34f487d4
src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java
---
a/src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 02:47:39 2019 +0100
+++
b/src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java
Thu Feb 28 16:48:19 2019 +0800
@@ -77,7 +77,7 @@
InputStream execute(String cmd, Object ... args)
throws AgentLoadException, IOException
{
- assert args.length <= 4; // includes null
+ assert args.length <= 3; // includes null
// create a pipe using a random name
Random rnd = new Random();
Thanks,
Lin
________________________________________
From: 臧琳
Sent: Thursday, February 28, 2019 3:24:52 PM
To: David Holmes; Yasumasa Suenaga
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
Subject: RE: Protocol version of Attach API
Hi David,
Since I don't have the access to JBS, may I ask your help
to ceate sub-
task? Thanks.
BRs,
Lin
-----Original Message-----
From: David Holmes <david.hol...@oracle.com>
Sent: Thursday, February 28, 2019 3:16 PM
To: 臧琳 <zangl...@jd.com>; Yasumasa Suenaga
<yasue...@gmail.com>
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
<serviceability-dev@openjdk.java.net>
Subject: Re: Protocol version of Attach API
Hi Lin,
On 28/02/2019 4:49 pm, 臧琳 wrote:
Hi David,
Your are right and thanks for pointing it out. when I worte
that patch, I
was considering implement -filename and -incremental together. and
I must be too stupid to forget recover it when I divided the
patch into
two.
And it seems a good solution is to refine the original
patch of jmap histo,
and try to composite all args as one when passing it to socket and
let attachlistener to handle the analyze.
I will try that.
One more, do I need to consider changing the jmap -dump
also?
I'm assuming -dump already works fine, so I'm just expecting to see
-histo handle the file in a similar manner.
If you find this works I suggest creating a sub-task of 8215622 to
first backout the original changes (hg backout), and a second
sub-task to REDO with the new implementation. Each will need
reviewing separately in their own RFR thread.
Thanks,
David
BRs,
Lin
________________________________________
From: David Holmes <david.hol...@oracle.com>
Sent: Thursday, February 28, 2019 12:59:28 PM
To: 臧琳; Yasumasa Suenaga
Cc: serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
Subject: Re: Protocol version of Attach API
Sorry I'm going to pick up on the rollback and re-do option here
as I just had a closer look at jmap. Given jmap -dump already has
more options than -histo does, why was any change to the "maximum
number of args" needed in the first place ???
David
On 28/02/2019 2:43 pm, David Holmes wrote:
Hi everyone,
I'm not sure we're converging on a suitable solution here, but to
address the issues flagged by Lin below ...
On 28/02/2019 12:39 pm, 臧琳 wrote:
Hi Suenaga,
Thanks for your expaination about the arg_length_max,
I generally agree with you that it is better to consider using
dynamic memory, and that would be handled carefully to aviod
introduce compatibility issue, plus it would be a big change. So
let’s see what others suggest.
Hi All,
It seems for me that there are basically three problems forked
by this
thread：
·Compatibility issue with old jcmd alike tools with
attachListener’s change.
This is issue:
https://bugs.openjdk.java.net/browse/JDK-8219721
·Only 3 arguments limited to passed by socket to attachListener
for Windows, which cause 8215622 work abnormally on Windows.
I have filed a new bug for this:
https://bugs.openjdk.java.net/browse/JDK-8219895
·The arg_length_max may not be enough for handling filename.
I have filed a new bug for this:
https://bugs.openjdk.java.net/browse/JDK-8219896
though it seems very related to this issue.
So I suggest we keep the first problem discussed in this thread,
and create separate thread for the others. Do you agree?
There is some overlap but yes this can be broken down somewhat -
though dealing with the variable length "packet" is going to have
to consider that what is received is in fact much larger than the
purported maximum packet size if these long paths are expected
and
accepted.
FWIW I don't see crashes or anything drastic if the arguments are
too long - the operations just fail (in somewhat obscure ways
sometimes).
For me, I will refine my patch to use timeout as a fix for the
first one, and update it in this thread. And I will try to fix
the second one for Windows, and create a separate thread for
discussing. And if possible, I can help to fix the third one.
What’s your opinion?
That sounds fine ...
Or you could choose to rollback JDK-8215622 and see how to solve
that without increasing the arg count. Given this usage:
jmap -histo:live,file=foo.txt <pid>
I'm not sure why this is sent to the VM as multiple args rather
than as a single composite arg that can then be parsed again by
the actual "jmap" logic. There would be some double-up perhaps if
the front-end tool wants to perform the command-line validation,
but it would be easy enough I think to do that checking then send
the original composite
arg.
Thanks,
David
-----
BRs,
Lin
*From:*Yasumasa Suenaga <yasue...@gmail.com>
*Sent:* Thursday, February 28, 2019 8:39 AM
*To:* 臧琳<zangl...@jd.com>
*Cc:* David Holmes <david.hol...@oracle.com>;
serviceability-dev@openjdk.java.net
serviceability-dev@openjdk.java.net
<serviceability-dev@openjdk.java.net>
*Subject:* Re: Protocol version of Attach API
2019年2月28日(木) 0:04 臧琳 <zangl...@jd.com
<mailto:zangl...@jd.com>>:
Dear Suenaga,
Thanks for your reviewing. I will try to refine
the patch.
For the argument length you mentioned, do you
mean the
"arg_length_max" should be large enough to accept the max
filename
length?
Yes, but it is not enough.
For example, jcmd VM.log might pass 2 or more paths to define
logs.
IMHO, all the handling of the argument length
is at receiver
side in the attachListener, such as
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot/os
/linux/att
achListener_linux.cpp#l322,
for me it means that the VM side limits the argments
length less
than arg_length_max, otherwise it will return NULL,
which cause
the
sender side (tools like jcmd and jmap) exits with
error message.
so
I think there may be no need to limit the argument
size in tool
side.
IMHO all programs which use filesystem should support any
locations on it.
So I think we should use dynamic memory (or GrowableArray) for
it if we do not change client side for compatibility.
And from my experiment with jmap, the arguments
send to
sockets are not arg0 only. as you can see in
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcm
d/sh
are/classes/sun/tools/jmap/JMap.java#l193
and
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcm
d/sh are/classes/sun/tools/jmap/JMap.java#l133,
jmap can pass arg0 as "filename", and arg1 as "-live",
and both
of
them can be NULL. so <ver>0<cmd>0<arg>0<arg>0<arg>0
can be
<ver>0<jmap>0<filename>0<live>0, and file can be null.
so 00
may not
indicate it reach the end.
We should consider for other tools - jstack and jinfo.
(jstack is ok because it will not have long arguments)
Thanks,
Yasumasa
BRs,
Lin
________________________________________
From: Yasumasa Suenaga <yasue...@gmail.com
<mailto:yasue...@gmail.com>>
Sent: Wednesday, February 27, 2019 8:10:14 PM
To: 臧琳
Cc: David Holmes; serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
Subject: Re: Protocol version of Attach API
Hi Lin,
I think we need to research more about this.
IMHO we need to match length of arguments between
server (AttachListener) and client (serviceability
tools) at least.
(please see previous email from me).
I have some comments for your change:
On 2019/02/27 18:22, 臧琳 wrote:
> Dear All,
> Here I have figured out one solution based on
timeout.
would
you like help to see whether this is OK?
> ---
a/src/hotspot/os/linux/attachListener_linux.cpp Tue Feb
26 14:57:23 2019 +0530
> +++
b/src/hotspot/os/linux/attachListener_linux.cpp Wed
Feb
27 17:21:48 2019 +0800
> @@ -263,9 +263,29 @@
> int off = 0;
> int left = max_len;
>
> + memset(buf, 0, max_len);
> + // set timeout for read
> + struct timeval timeout;
> + timeout.tv_sec = 3;
> + timeout.tv_usec = 0;
I think timeout value should be configurable.
For example, we can introduce new flag in globals.hpp .
> + if(setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, (struct
timeval*)&timeout, sizeof(timeout))) {
> + log_debug(attach)("Failed to set socket option
SO_RCVTIMEO:
%s\n", strerror(errorno));
> + printf("Warning: Failed to set socket option
SO_RCVTIMEO:
%s!!!\n", strerror(errno));
You should not use printf(), and do you need to pass
'\n' to
log_debug()?
> + }
> do {
> int n;
> - RESTARTABLE(read(s, buf+off, left), n);
> + log_debug(attach)("start reading data from
socket....\n");
> + n = read(s, buf+off, left);
You should use RESTARTABLE macro.
read(2) might be interrupted by signal.
> + if (n < 0) {
> + if (errno == EWOULDBLOCK) {
According to man page, read(2) sets EWOULDBLOCK or
EAGAIN.
So you should check both errno.
> + for (int i = str_count; i <
expected_str_count; i++) {
> + //timeout, fill reminded arguments with
\0;
> + buf[off+i] = '\0';
> + str_count++;
> + }
You set zero to buf[] in above.
So you can remove this loop, and set str_count to
expected_str_count
without manipulating buf[].
In addition, I prefer to add log_debug() at this
to record NULL arguments are added.
> + break;;
> + }
> + }
> assert(n <= left, "buffer was too small,
impossible!");
> buf[max_len - 1] = '\0';
> if (n == -1) {
Thanks,
Yasumasa
> Thanks.
> Lin
>
> ________________________________________
> From: Yasumasa Suenaga <yasue...@gmail.com
<mailto:yasue...@gmail.com>>
> Sent: Wednesday, February 27, 2019 15:15
> To: David Holmes; 臧琳
> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
> Subject: Re: Protocol version of Attach API
>
> On 2019/02/27 15:59, David Holmes wrote:
>> On 27/02/2019 4:10 pm, Yasumasa Suenaga wrote:
>>> Hi,
>>>
>>> Buffer size for receiving packets from client is
determined at [1].
>>
>> Maximum buffer size, yes.
>>
>>> It defines length of command name and of argument.
>>> It is passed via Unix domain, so we fill '\0' to
remaining
bytes and
>>> might be able to assume all arguments are passed
with empty string.
>>
>> Not sure what you mean.
>>
>> // The buffer is expected to be formatted as follows:
>> // <ver>0<cmd>0<arg>0<arg>0<arg>0
>>
>> so we can expect to read at least two things - the
ver and
cmd.
If we encounter 00 we can infer we reached the end.
But we may
not
have read the full data into the buffer, so can't tell
if another
call to read() is needed yet - you only know after you've
read the 00.
>>
>>> BTW length of arguments is defined to 1024 in [2].
>>> jcmd and jmap might pas file path - it might be
JVM_MAXPATHLEN
(4097 bytes).
>>> Buffer size which is used in AttachListener seems
not to be enough.
>>
>> One has to assume/hope that the code sending the
data is
working
to the same protocol rules as the receiver! Otherwise
this is just
completely broken.
>
> On Linux, client (VirtualMachineImpl) seems not to
check
length
of arguments:
>
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.att
ach/
linux/classes/sun/tools/attach/VirtualMachineImpl.java#l168
>
> In case of jcmd, all options are passed to arg #1.
It seems not
to check the length:
>
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcm
d/sh
are/classes/sun/tools/jcmd/JCmd.java#l111
>
>
> I guess other tools (jstack, jmap, etc) which
connect to
AttachListener are same.
> So we can fix both Attach API and AttachListener
(it will be big
change!),
> but I concern we can keep protocol version...
>
>
> Thanks,
>
> Yasumasa
>
>
>> David
>> -----
>>
>>> We might have to change more.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> [1]
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot
/os/
linux/attachListener_linux.cpp#l254
>>> [2]
http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/hotspot
/sha
re/services/attachListener.hpp#l106
>>>
>>>
>>> On 2019/02/27 15:00, 臧琳 wrote:
>>>> Another solution I can figure out is try to add
timeout for
socket read. I will also investigate whether is works.
>>>>
>>>> Cheers,
>>>> Lin
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: 臧琳
>>>>> Sent: Wednesday, February 27, 2019 1:52 PM
>>>>> To: 'David Holmes' <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>; Yasumasa Suenaga
>>>>> <yasue...@gmail.com <mailto:yasue...@gmail.com>>
>>>>> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
>>>>> Subject: RE: Protocol version of Attach API
>>>>>
>>>>> Dear David, Yasumasa,
>>>>> I think it is hard to know how long the
buffer is
passed from socket
>>>>> without changing the info of the message from the
receiver side.
>>>>> So how about when str_count reach to 3,
we test it
with non_blocking
>>>>> read?
>>>>>
>>>>>
>>>>> Cheers,
>>>>> Lin
>>>>>> -----Original Message-----
>>>>>> From: David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>>
>>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>>>>> To: Yasumasa Suenaga <yasue...@gmail.com
<mailto:yasue...@gmail.com>>; 臧琳 <zangl...@jd.com
<mailto:zangl...@jd.com>>
>>>>>> Cc: serviceability-dev@openjdk.java.net
<mailto:serviceability-dev@openjdk.java.net>
>>>>>> Subject: Re: Protocol version of Attach API
>>>>>>
>>>>>> Hi Yasumasa,
>>>>>>
>>>>>> On 27/02/2019 1:05 pm, Yasumasa Suenaga wrote:
>>>>>>> Hi Lin,
>>>>>>>
>>>>>>> My proposal is a just idea, so you need to
tweak it.
>>>>>>>
>>>>>>> AttachListener receives raw command.
>>>>>>> For example, jcmd is `jcmd\0<arg strings>`.
Please see
>>>>>>> HotSpotVirtualMachine and extended classes.
>>>>>>>
>>>>>>> In case of jcmd, I guess AttachListener will
receive
message
>>>>>>> `<version>\0jcmd\0<args>\0\0\0` (I did not
check it
well).
>>>>>>> So I guess we can add '\0' when `str_count`
does not
reach
to maximum.
>>>>>>
>>>>>> I don't see how this approach can work. You have
to know
how
many
>>>>>> arguments are coming in the "packet", but that
information
is not
>>>>>> available in the current Linux
implementation.Without it
you
can't
>>>>>> know when to stop calling read().
>>>>>>
>>>>>> The protocol would need to be changed to send the
"packet"
size, but
>>>>>> that's not compatible with older JDKs.
>>>>>>
>>>>>> Otherwise I think we have no choice but to use a
non-blocking read ...
>>>>>> though I'm still unsure if you can know for
certain that you've
>>>>>> reached the end of the "packet" ...
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>>
>>>>>>>
>>>>>>> On 2019/02/27 11:50, zangl...@jd.com
<mailto:zangl...@jd.com> wrote:
>>>>>>>> Dear Yasumasa,
>>>>>>>> The fix you mentioned seems not work as
expected.
>>>>>>>> I have done an experiment that use
jdk1.8's "jcmd
<pid> help" to
>>>>>>>> attach to latest jdk.
>>>>>>>> it seem the whole "jcmd <pid> help"
buffer is not
>>>>>>>> read completely at one time. in my case
it is "jcmd",
"<pid>",
>>>>>>>> "help", and then block at while().
>>>>>>>> After applied your change, it seems only
the "jcmd"
is
>>>>>>>> processed, the "<pid>", "help" is replaced
by '\0'.
>>>>>>>>
>>>>>>>> BRs,
>>>>>>>> Lin
>>>>>>>>
