RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix
Gary Adams
gary.adams at oracle.com
Tue Dec 19 12:47:35 UTC 2017
On 12/18/17, 4:38 PM, Chris Plummer wrote:
> On 12/18/17 1:22 PM, gary.adams at 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
>
>
So what I'm hearing is that we should fix more than just the error
message while we're changing this code. If findSocketFile returns the File,
it is easy enough for the calling code to handle the calls to exists and
getPath
without overloading path to be null as a returned flag. No need to
recompute the file
name each time through the loop. e.g.
diff --git
a/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
--- a/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
+++ b/src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights
reserved.
* Copyright (c) 2015 SAP SE. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
@@ -47,9 +47,6 @@
// Any changes to this needs to be synchronized with HotSpot.
private static final String tmpdir = "/tmp";
- // The patch to the socket file created by the target VM
- String path;
-
/**
* Attaches to the target VM
*/
@@ -69,8 +66,9 @@
// Find the socket file. If not found then we attempt to start the
// attach mechanism in the target VM by sending it a QUIT signal.
// Then we attempt to find the socket file again.
- path = findSocketFile(pid);
- if (path == null) {
+ File path = new File(tmpdir, ".java_pid" + pid);
+ String socket_name = path.getPath();
+ if (!path.exists()) {
File f = createAttachFile(pid);
try {
sendQuitTo(pid);
@@ -86,19 +84,18 @@
try {
Thread.sleep(delay);
} catch (InterruptedException x) { }
- path = findSocketFile(pid);
time_spend += delay;
- if (time_spend > timeout/2 && path == null) {
+ if (time_spend > timeout/2 && !path.exists()) {
// Send QUIT again to give target VM the last
chance to react
sendQuitTo(pid);
}
- } while (time_spend <= timeout && path == null);
- if (path == null) {
+ } while (time_spend <= timeout && !path.exists());
+ if (!path.exists()) {
throw new AttachNotSupportedException(
String.format("Unable to open socket file %s: " +
"target process %d doesn't respond within
%dms " +
- "or HotSpot VM not loaded", f.getPath(), pid,
time_spend));
+ "or HotSpot VM not loaded", socket_name, pid,
time_spend));
}
} finally {
f.delete();
@@ -107,14 +104,14 @@
// Check that the file owner/permission to avoid attaching to
// bogus process
- checkPermissions(path);
+ checkPermissions(socket_name);
// Check that we can connect to the process
// - this ensures we throw the permission denied error now
rather than
// later when we attempt to enqueue a command.
int s = socket();
try {
- connect(s, path);
+ connect(s, socket_name);
} finally {
close(s);
}
@@ -264,15 +261,6 @@
}
}
- // Return the socket file for the given process.
- private String findSocketFile(int pid) {
- File f = new File(tmpdir, ".java_pid" + pid);
- if (!f.exists()) {
- return null;
- }
- return f.getPath();
- }
-
// On Solaris/Linux/Aix a simple handshake is used to start the
attach mechanism
// if not already started. The client creates a .attach_pid<pid>
file in the
// target VM's working directory (or temp directory), and the
SIGQUIT handler
More information about the serviceability-dev
mailing list