Protocol version of Attach API
Yasumasa Suenaga
yasuenag at gmail.com
Fri Mar 1 03:54:54 UTC 2019
Hi,
I agree with David. I think we should backout 8215622.
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