RFR(10)(M): 8179498: attach in linux should be relative to /proc/pid/root and namespace aware

[David Holmes]

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, serguei.spitsyn at oracle.com 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
>>>>>
>>>>
>>>
> 
>