[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible o
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2017-12-22 19:25:28
Message-ID: b5a6ce5e-6777-5ce3-af67-9099f1490759 () oracle ! com
[Download RAW message or body]

Gary,

The fix looks good.
Thank you for the update!


On 12/22/17 07:48, Gary Adams wrote:
> A refreshed webrev is available
>        Webrev: http://cr.openjdk.java.net/~gadams/8188856/webrev.03/
>
>   - "String socket_path" is now used where the original "path" variable 
> was used
>   - "File socket_file" is used for the exists() and getPath() calls
>   - left solaris openDoor optimizations out, because it is not really 
> related
>        to the original reported issue
>   - left linux findSocketFile as a separate method, because it contains
>        extra nspid cgroup aware logic. The original findSocketFile performed
>        2 steps to formulate the java_pid path name and to check existence.
>        Now that it only formulates the path name, it seems reasonable to 
> fold the
>        processing into the caller. There already was an asymmetry in the 
> platform
>        specific implementations as solaris was using openDoor and the 
> open file
>        descriptor for it's connected flag checks.

I was suggesting to also keep findSocketFile for aix and macosx to keep 
it unified with linux.
Now I see it does not have much sense as the findSocketFile becomes too 
trivial for aix and macosx.

Thanks,
Serguei


> Bypassed the mach5 errors I was seeing by moving over to testing with
> jdk/jdk repos.
>
> If this is acceptable, I'll create an hg export patch file for
> my sponsor.
>
> On 12/21/17, 3:17 PM, Gary Adams wrote:
>> A refreshed webrev is available
>>        Webrev: http://cr.openjdk.java.net/~gadams/8188856/webrev.02/
>>
>>    - added solaris version of the file which had the same original 
>> error reporting
>>          the wrong socket file name
>>    - restored the linux detach/execute logic
>>    - updated the macosx and aix detach/execute logic
>>    - a few minor differences between the various platform specific files
>>          curly braces and exception args.
>>
>> Ran into some issues with mach5 testing which will require another
>> test run tomorrow.
>>
>> On 12/19/17, 10:00 AM, Gary Adams wrote:
>>> A refreshed webrev is available
>>>        Webrev: http://cr.openjdk.java.net/~gadams/8188856/webrev.01/
>>>
>>> On 12/18/17, 4:38 PM, Chris Plummer wrote:
>>>> On 12/18/17 1:22 PM, gary.adams@oracle.com wrote:
>>>>> On 12/18/17 2:26 PM, Chris Plummer wrote:
>>>>>> Hi Gary,
>>>>>>
>>>>>> On 12/18/17 6:47 AM, Gary Adams wrote:
>>>>>>> Here's a simple fix to correct the error message when the 
>>>>>>> java_pid socket
>>>>>>> is not found. The code previously reported the attach_pid file name
>>>>>>> rather than the socket file name that was not found.
>>>>>>>
>>>>>>>    Issue: https://bugs.openjdk.java.net/browse/JDK-8188856
>>>>>>>    Webrev: http://cr.openjdk.java.net/~gadams/8188856/webrev.00/
>>>>>>
>>>>>> I don't understand why you couldn't simply have changed the 
>>>>>> f.getPath() reference to "path". From what I can see, there is no 
>>>>>> difference between "path" and "socket_name". The problem you are 
>>>>>> fixing is that the error message prints f.getPath(), but that 
>>>>>> refers to the attach file and the error message should refer to 
>>>>>> the socket file. You've correct this, but have done so in a round 
>>>>>> about way. Above the error message (in two places) exists:
>>>>>>
>>>>>>                      path = findSocketFile(pid, ns_pid);
>>>>>>
>>>>>> So "path" is what you want. You have indirectly fixed the problem 
>>>>>> by having findSocketFile() store the path in socket_name, and 
>>>>>> then you print socket_name, but why not just do the direct fix 
>>>>>> and print "path".
>>>>>>
>>>>>> Also, the copyrights need to be updated.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>>
>>>>> The problem was path is used to hold the constructed file name
>>>>> if it is confirmed to exist in the file system. Otherwise it is 
>>>>> passed back from
>>>>> findSocketFile as a null to indicate the socket file was not found.
>>>>>
>>>>> I could refactor where the existence check is handled, but it 
>>>>> seemed like the least
>>>>> invasive change to simply save the socket name for the printing in 
>>>>> the error case.
>>>>>
>>>> Ah, right. Obviously "path" is null when you want to print the 
>>>> error message. I guess I   have a hard time with "path" and 
>>>> "socket_name" for the most part being the same (except "path" can 
>>>> end up being null), yet they have two completely dissimilar names. 
>>>> Why not rename path to socket_path and then add saved_socket_path, 
>>>> or something like that. No changes to your current logic, just to 
>>>> the names being used.
>>>>
>>>> Or, have findSocketFile() actually return the File, and then rename 
>>>> path to socket_file and also rename socket_name to socket_path. The 
>>>> truth of the matter is the result of findSocketFile() is only used 
>>>> to see if the socket file exists. You could even change it to 
>>>> return a boolean.
>>>>
>>>> Chris
>>>>
>>>>
>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]