Protocol version of Attach API
臧琳
zanglin5 at jd.com
Thu Feb 28 07:24:52 UTC 2019
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