Protocol version of Attach API
David Holmes
david.holmes at oracle.com
Fri Mar 1 04:22:14 UTC 2019
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.jcmd/sh
>>>>>>> are/classes/sun/tools/jmap/JMap.java#l193
>>>>>>>
>>>>>>> and
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/df3d253aaf81/src/jdk.jcmd/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.attach/
>>>>>>> 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.jcmd/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