Protocol version of Attach API
David Holmes
david.holmes at oracle.com
Mon Mar 4 07:34:10 UTC 2019
PS. This should be sent out in a proper RFR thread for JDK-8219721
Thanks,
David
On 4/03/2019 5:22 pm, David Holmes wrote:
> 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