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 臧琳 <[email protected]>: > > 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 <[email protected]> > Sent: Thursday, February 28, 2019 7:55:01 PM > To: 臧琳; Yasumasa Suenaga > Cc: [email protected] [email protected] > 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: [email protected] [email protected] > > 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: [email protected] [email protected] > > 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 <[email protected]> > >> Sent: Thursday, February 28, 2019 3:16 PM > >> To: 臧琳 <[email protected]>; Yasumasa Suenaga <[email protected]> > >> Cc: [email protected] [email protected] > >> <[email protected]> > >> 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 <[email protected]> > >>> Sent: Thursday, February 28, 2019 12:59:28 PM > >>> To: 臧琳; Yasumasa Suenaga > >>> Cc: [email protected] > >>> [email protected] > >>> 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 <[email protected]> > >>>>> *Sent:* Thursday, February 28, 2019 8:39 AM > >>>>> *To:* 臧琳<[email protected]> > >>>>> *Cc:* David Holmes <[email protected]>; > >>>>> [email protected] > >>>>> [email protected] > >>>>> <[email protected]> > >>>>> *Subject:* Re: Protocol version of Attach API > >>>>> > >>>>> 2019年2月28日(木) 0:04 臧琳 <[email protected] > >> <mailto:[email protected]>>: > >>>>> > >>>>> 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 <[email protected] > >>>>> <mailto:[email protected]>> > >>>>> Sent: Wednesday, February 27, 2019 8:10:14 PM > >>>>> To: 臧琳 > >>>>> Cc: David Holmes; [email protected] > >>>>> <mailto:[email protected]> > >>>>> 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 <[email protected] > >>>>> <mailto:[email protected]>> > >>>>> > Sent: Wednesday, February 27, 2019 15:15 > >>>>> > To: David Holmes; 臧琳 > >>>>> > Cc: [email protected] > >>>>> <mailto:[email protected]> > >>>>> > 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' <[email protected] > >>>>> <mailto:[email protected]>>; Yasumasa Suenaga > >>>>> >>>>> <[email protected] <mailto:[email protected]>> > >>>>> >>>>> Cc: [email protected] > >>>>> <mailto:[email protected]> > >>>>> >>>>> 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 <[email protected] > >>>>> <mailto:[email protected]>> > >>>>> >>>>>> Sent: Wednesday, February 27, 2019 1:44 PM > >>>>> >>>>>> To: Yasumasa Suenaga <[email protected] > >>>>> <mailto:[email protected]>>; 臧琳 <[email protected] > >>>>> <mailto:[email protected]>> > >>>>> >>>>>> Cc: [email protected] > >>>>> <mailto:[email protected]> > >>>>> >>>>>> 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, [email protected] > >>>>> <mailto:[email protected]> 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 > >>>>> >>>>>>>> > >>>>>
