Thanks David, Here is the RFR thread https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027337.html
Cheers, Lin ________________________________________ From: David Holmes <david.hol...@oracle.com> Sent: Monday, March 4, 2019 3:34:10 PM To: 臧琳; Yasumasa Suenaga; Hohensee, Paul Cc: serviceability-dev@openjdk.java.net serviceability-dev@openjdk.java.net Subject: Re: Protocol version of Attach API PS. This should be sent out in a proper RFR thread for JDK-8219721 Thanks, David On 4/03/2019 5:22 pm, David Holmes wrote: > 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 >>>>>>>>>> >>>>>>>> >>>>>>>>>>
