RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
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/
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

serguei.spitsyn@oracle.com
Hi Gary,

It looks good to me.

Thanks,
Serguei


On 12/18/17 06:47, 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/

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Chris Plummer
In reply to this post by Gary Adams-3
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


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
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.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Chris Plummer
On 12/18/17 1:22 PM, [hidden email] 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


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
On 12/18/17, 4:38 PM, Chris Plummer wrote:

> On 12/18/17 1:22 PM, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
In reply to this post by Chris Plummer
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, [hidden email] 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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Chris Plummer
Hi Gary,

I'm not sure about the detach() and execute() changes on linux (how can
this.path references just be stripped and the code still work properly),
and how does this code continue to work on AIX and MacOSX when "path"
has been removed but is still referenced. Shouldn't this.path just be
replaced with this.socket_name?

thanks,

Chris

On 12/19/17 7: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, [hidden email] 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
>>
>>
>


Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
More work is needed here. I have not tracked down how detach is actually
used.
The previous path as null string was a flag to reconnect (?).

On 12/19/17 5:23 PM, Chris Plummer wrote:

> Hi Gary,
>
> I'm not sure about the detach() and execute() changes on linux (how
> can this.path references just be stripped and the code still work
> properly), and how does this code continue to work on AIX and MacOSX
> when "path" has been removed but is still referenced. Shouldn't
> this.path just be replaced with this.socket_name?
>
> thanks,
>
> Chris
>
> On 12/19/17 7: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, [hidden email] 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
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
I've done a little bit more investigation to learn how detach and execute
are expected to work. Looking at the solaris implementation, detach
actually closes
the file descriptor and the check in execute is is fd == -1.
In the macosx implementation the check uses path == null.

So to correct the changeset in progress, I need to restore
the flag indicating that a connection has been made and the
detach case which clears the current connection.

Let me run that through some instrumentation tests cases
and come back with the next webrev tomorrow.

On 12/19/17, 5:46 PM, [hidden email] wrote:

> More work is needed here. I have not tracked down how detach is
> actually used.
> The previous path as null string was a flag to reconnect (?).
>
> On 12/19/17 5:23 PM, Chris Plummer wrote:
>> Hi Gary,
>>
>> I'm not sure about the detach() and execute() changes on linux (how
>> can this.path references just be stripped and the code still work
>> properly), and how does this code continue to work on AIX and MacOSX
>> when "path" has been removed but is still referenced. Shouldn't
>> this.path just be replaced with this.socket_name?
>>
>> thanks,
>>
>> Chris

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
In reply to this post by Gary Adams-3
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, [hidden email] 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
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

serguei.spitsyn@oracle.com
Hi Gary,

+        File path = new File(tmpdir, ".java_pid" + pid);
The path is better to rename to file now.
Otherwise, it is a source of confusion.


A couple of minor comments on the:
  http://cr.openjdk.java.net/~gadams/8188856/webrev.02/open/src/jdk.attach/solaris/classes/sun/tools/attach/VirtualMachineImpl.java.frames.html

 A copyright comment needs an update.
 This was before your update, but better to fix as it is touched: ';;' at the end:
   214 socket_path = tmpdir + "/.java_pid" + pid;;

 Also, string concatenation is expensive now.
 Might be better to pass a socket_path to openDoor instead of a pid.


Thanks,
Serguei


On 12/21/17 12:17, 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, [hidden email] 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





Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

serguei.spitsyn@oracle.com
One more suggestion.
I like the linux variant which keeps the findSocketFile() method.
Would it better to unify things and do the same for aix and macosx?

Thanks,
Serguei


On 12/21/17 15:37, [hidden email] wrote:
Hi Gary,

+        File path = new File(tmpdir, ".java_pid" + pid);
The path is better to rename to file now.
Otherwise, it is a source of confusion.


A couple of minor comments on the:
  http://cr.openjdk.java.net/~gadams/8188856/webrev.02/open/src/jdk.attach/solaris/classes/sun/tools/attach/VirtualMachineImpl.java.frames.html

 A copyright comment needs an update.
 This was before your update, but better to fix as it is touched: ';;' at the end:
   214 socket_path = tmpdir + "/.java_pid" + pid;;

 Also, string concatenation is expensive now.
 Might be better to pass a socket_path to openDoor instead of a pid.


Thanks,
Serguei


On 12/21/17 12:17, 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, [hidden email] 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






Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

Gary Adams-3
In reply to this post by Gary Adams-3
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.

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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8188856: Incorrect file path in an exception message when .java_pid is not accessible on Unix

serguei.spitsyn@oracle.com
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, [hidden email] 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
>>>>
>>>>
>>>
>>
>