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