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.
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.
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
