Protocol version of Attach API
臧琳
zanglin5 at jd.com
Mon Mar 4 07:30:47 UTC 2019
Thanks David,
I will update the webrev for the fix by EOD today. Then we can review it.
I agree with you for more work to do for the args, and that seems the precondition for me to move forward for other enhancement of the jmap/jcmd alike tools. I will try to investigate it.
BRs,
Lin
________________________________________
From: David Holmes <david.holmes at oracle.com>
Sent: Monday, March 4, 2019 3:22:34 PM
To: 臧琳; Yasumasa Suenaga; Hohensee, Paul
Cc: serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net
Subject: Re: Protocol version of Attach API
Hi Lin,
I think this is fine to address the problem that was introduced.
There's more to be done in this area as there is obviously a
misunderstanding about the "args" expected in the "packet" versus the
'args' for any particular command.
With this change we can close JDK-8219895 as "Not an issue".
Thanks,
David
On 1/03/2019 7:22 pm, 臧琳 wrote:
> Dear All,
> I have upload a webrev at http://cr.openjdk.java.net/~xiaofeya/8219721/webrev.00/ (just found there is a jank file "abc", I will omit it in next webrev, but let's review other changes first for time saving.)
> And here are my comments and questions:
> * With this patch , I have tested with jdk8/jdk11's jcmd/jmap, all works as expected
> * And passed tier1 test on my linux box
> * Besides change from 4 to 3 , I also found one compatibility issue of using old "jmap" like "jmap -histo:live", caused by the sequence of arguments for inspectheap. And I have include the fix in the webrev. What I am concerned is that is this trivial enough to avoid revert and redo. IMO, if you think it shoud be revert 8215622 with this change, please let me know.
>
> Thanks for all of your help and suggestions.
>
> BRs,
> Lin
>
>> -----Original Message-----
>> From: David Holmes <david.holmes at oracle.com>
>> Sent: Friday, March 1, 2019 12:22 PM
>> To: Yasumasa Suenaga <yasuenag at gmail.com>; 臧琳 <zanglin5 at jd.com>
>> Cc: serviceability-dev at openjdk.java.net serviceability-dev at openjdk.java.net
>> <serviceability-dev at 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 臧琳 <zanglin5 at 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.holmes at oracle.com>
>>>> Sent: Thursday, February 28, 2019 7:55:01 PM
>>>> To: 臧琳; Yasumasa Suenaga
>>>> Cc: serviceability-dev at openjdk.java.net
>>>> serviceability-dev at 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 at openjdk.java.net
>>>>> serviceability-dev at 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 at openjdk.java.net
>>>>> serviceability-dev at 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.holmes at oracle.com>
>>>>>> Sent: Thursday, February 28, 2019 3:16 PM
>>>>>> To: 臧琳 <zanglin5 at jd.com>; Yasumasa Suenaga
>> <yasuenag at gmail.com>
>>>>>> Cc: serviceability-dev at openjdk.java.net
>>>>>> serviceability-dev at openjdk.java.net
>>>>>> <serviceability-dev at 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.holmes at oracle.com>
>>>>>>> Sent: Thursday, February 28, 2019 12:59:28 PM
>>>>>>> To: 臧琳; Yasumasa Suenaga
>>>>>>> Cc: serviceability-dev at openjdk.java.net
>>>>>>> serviceability-dev at 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 <yasuenag at gmail.com>
>>>>>>>>> *Sent:* Thursday, February 28, 2019 8:39 AM
>>>>>>>>> *To:* 臧琳<zanglin5 at jd.com>
>>>>>>>>> *Cc:* David Holmes <david.holmes at oracle.com>;
>>>>>>>>> serviceability-dev at openjdk.java.net
>>>>>>>>> serviceability-dev at openjdk.java.net
>>>>>>>>> <serviceability-dev at openjdk.java.net>
>>>>>>>>> *Subject:* Re: Protocol version of Attach API
>>>>>>>>>
>>>>>>>>> 2019年2月28日(木) 0:04 臧琳 <zanglin5 at jd.com
>>>>>> <mailto:zanglin5 at 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 <yasuenag at gmail.com
>>>>>>>>> <mailto:yasuenag at gmail.com>>
>>>>>>>>> Sent: Wednesday, February 27, 2019 8:10:14 PM
>>>>>>>>> To: 臧琳
>>>>>>>>> Cc: David Holmes; serviceability-dev at openjdk.java.net
>>>>>>>>> <mailto:serviceability-dev at 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 <yasuenag at gmail.com
>>>>>>>>> <mailto:yasuenag at gmail.com>>
>>>>>>>>> > Sent: Wednesday, February 27, 2019 15:15
>>>>>>>>> > To: David Holmes; 臧琳
>>>>>>>>> > Cc: serviceability-dev at openjdk.java.net
>>>>>>>>> <mailto:serviceability-dev at 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.holmes at oracle.com
>>>>>>>>> <mailto:david.holmes at oracle.com>>; Yasumasa Suenaga
>>>>>>>>> >>>>> <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>
>>>>>>>>> >>>>> Cc: serviceability-dev at openjdk.java.net
>>>>>>>>> <mailto:serviceability-dev at 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.holmes at oracle.com
>>>>>>>>> <mailto:david.holmes at oracle.com>>
>>>>>>>>> >>>>>> Sent: Wednesday, February 27, 2019 1:44 PM
>>>>>>>>> >>>>>> To: Yasumasa Suenaga <yasuenag at gmail.com
>>>>>>>>> <mailto:yasuenag at gmail.com>>; 臧琳 <zanglin5 at jd.com
>>>>>>>>> <mailto:zanglin5 at jd.com>>
>>>>>>>>> >>>>>> Cc: serviceability-dev at openjdk.java.net
>>>>>>>>> <mailto:serviceability-dev at 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, zanglin5 at jd.com
>>>>>>>>> <mailto:zanglin5 at 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
>>>>>>>>> >>>>>>>>
>>>>>>>>>
More information about the serviceability-dev
mailing list