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, [email protected] 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
