I think the /tmp fallback is there because cwd might not be writeable by the application user. (In fact I am not sure why it does not insist on /tmp in the first place as that one needs to be writeable for the attach to work anyway. It is certainly less surprising to create a file there instead of a random cwd of another process.
Gruss Bernd -- http://bernd.eckenfels.net ________________________________ From: David Holmes <[email protected]> Sent: Thursday, September 14, 2017 7:36:41 AM To: Chris Plummer; [email protected]; serviceability-dev; [email protected]; Bernd Eckenfels Subject: Re: RFR(10)(M): 8179498: attach in linux should be relative to /proc/pid/root and namespace aware On 14/09/2017 2:15 PM, Chris Plummer wrote: > On 9/13/17 9:00 PM, David Holmes wrote: >> Hi Chris, >> >> On 14/09/2017 1:03 PM, Chris Plummer wrote: >>> Hi David, >>> >>> On 9/13/17 5:12 PM, David Holmes wrote: >>>> Hi Chris, >>>> >>>> On 14/09/2017 8:23 AM, Chris Plummer wrote: >>>>> I could use one more reviewer. >>>> >>>> Generally this seems okay to me. >>>> >>>> One query though ... in createAttachFile don't you need to alter the >>>> tmpdir using part in a similar manner to how findSocketFile was >>>> modified? >>> The fix in findSocketFile is not just to make sure the client uses >>> the correct pid in the .java_pid file files, but also (as you point >>> out) to make sure that the client properly references the target >>> jvm's tmp directory when accessing the .java_pid file. findSocketFile >>> is a little >> >> I presume you mean createAttachFile there. > Yes. >> >>> different. You still have to map to the proper from pid to ns_pid >>> when referencing the .attach_pid file, but you don't have the /tmp >>> mount point differences to deal with. /proc/<pid>/cwd should work >>> even if the pid is for a docker. You don't even have to map to the >>> pid as the docker sees it. /proc/<pid>/cwd from the client's POV >>> should be the same as /proc/<ns_pid>/cwd from the target JVM's POV. >> >> Sorry but I don't follow. If findSocketFile has to look in >> /proc/<pid>/root/<tmpdir> for the socket file, why does the >> createAttachFile not also have to write the attach file into >> /proc/<pid>/root/<tmpdir> ?? In both cases it needs to find the tmpdir >> of the target process. > Fortunately I have some old printlns that might help: > > ***getNamespacePid - ns_pid: 125 > ***findSocketFile - f: /proc/24445/root/tmp/.java_pid125 > ***createAttachFile - path: /proc/24445/cwd/.attach_pid125 > > So this is a case where the real pid is 24445, but the namespace pid in > the docker is 125. The docker can (and does) reference > /tmp/.java_pid125, but the client needs to reference > /proc/24445/root/tmp/.java_pid125 to get to the same file. For > .attach_pid125, the client can get to it through > /proc/24445/cwd/.attach_pid125, and the docker process will look in cwd > for the file. This is done in AttachListener::is_init_trigger(). That's fine it isn't the cwd code path at issue. > BTW, comments like the following are no longer correct due to JDK-7132199: > > // "/tmp" is used as a global well-known location for the files > // .java_pid<pid>. and .attach_pid<pid>. It is important that this > // location is the same for all processes, otherwise the tools > // will not be able to find all Hotspot processes. > // Any changes to this needs to be synchronized with HotSpot. > private static final String tmpdir = "/tmp"; > > So now .attach_pid<pid> is always created in cwd as you can see in > createAttachFile(), although AttachListener::is_init_trigger() does > check tmp, but only after cwd. 294 } catch (IOException x) { 295 f = new File(tmpdir, fn); 296 f.createNewFile(); 297 } This is the backup in case cwd fails for some reason. I have no idea why it might fail but either: a) it can fail in which case I still think the tmpfile case needs to be updated; or b) it can't fail, in which case the code above should be removed along with any other code that checks in tmpdir. Thanks, David > thanks, > > Chris >> >> Thanks, >> David >> >>>> >>>> Minor note - you can collapse your catch blocks into 1 using >>>> something like >>>> >>>> ������� } catch (NumberFormatException | IOException x) { >>>> ������������ throw new AttachNotSupportedException("Unable to parse >>>> namespace"); >>>> ������� } >>> I'll make that change. >>> >>> thanks, >>> >>> Chris >>>> >>>> Cheers, >>>> David >>>>> thanks, >>>>> >>>>> Chris >>>>> >>>>> On 9/11/17 8:03 PM, Chris Plummer wrote: >>>>>> Ok, I will. Thanks. >>>>>> >>>>>> Chris >>>>>> >>>>>> On 9/11/17 6:13 PM, [email protected] wrote: >>>>>>> Hi Chris, >>>>>>> >>>>>>> This looks good to me. >>>>>>> >>>>>>> I'm not sure if all the nsk.aod and the AttachOnDemand tests from >>>>>>> the nsk.jvmti are run in the hotspot tier1, 2, and 3 tests. >>>>>>> It makes sense to double-check it. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Serguei >>>>>>> >>>>>>> >>>>>>> On 9/10/17 20:34, Chris Plummer wrote: >>>>>>>> [re-sending - sent to wrong alias first time] >>>>>>>> >>>>>>>> Hello, >>>>>>>> >>>>>>>> Please review the following: >>>>>>>> >>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8179498 >>>>>>>> http://cr.openjdk.java.net/~cjplummer/8179498/webrev.00/webrev_jdk/ >>>>>>>> >>>>>>>> The CR has the relevant details. Some previous discussions can >>>>>>>> be found here: >>>>>>>> >>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-April/021237.html >>>>>>>> >>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-May/021249.html >>>>>>>> >>>>>>>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-August/021679.html >>>>>>>> >>>>>>>> >>>>>>>> Testing with docker has been limited to just making sure jcmd >>>>>>>> now works with the docker setup I was provided. I currently >>>>>>>> don't see how we can run our existing tests in a way that would >>>>>>>> test the docker support without doing some rewriting of the tests. >>>>>>>> >>>>>>>> I also ran all our hotspot tier1, 2, and 3 tests, along with >>>>>>>> jdk/test/tools and jdk/test/sun/tools tests to make sure >>>>>>>> existing functionality is not broken with these changes. >>>>>>>> >>>>>>>> thanks, >>>>>>>> >>>>>>>> Chris >>>>>>> >>>>>> >>>>> >>> >>> > >
