RFR (M/L): 8131168: Refactor ProcessHandleImpl_*.c and add implememtation for AIX

Roger Riggs Roger.Riggs at Oracle.com
Fri Aug 7 18:54:36 UTC 2015


Hi Volker,

1) ProcessHandle.java:243

For the definition of the new commandLine method I would:

   - Use @link instead of @code for references to commands() and 
arguments() for easy navigation

   - @implNote[1] should I think be changed to @apiNote:
   the text describes not the JDK implementation but is information 
about the
   information returned and is use to the application developer, not the 
JDK implementation.

   -  The specific references to Linux implementation command line 
length parameters seems out of place
       and should be omitted.

         /**
          * Returns the command line of the process.
          * <p>
          * If {@link #command command()} and  {@link #arguments 
arguments()} return non-null
          * optionals, this is simply a convenience method which 
concatenates
          * the values of the two functions separated by spaces. 
Otherwise it will return a
          * best-effort, platform dependent representation of the 
command line.
          *
          * @apiNote Note that the returned executable pathname and the
          *           arguments may be truncated on some platforms due 
to system
          *           limitations.
          *           <p>
          *           The executable pathname may contain only the
          *           name of the executable without the full path 
information.
          *           It is undecideable whether white space separates 
different
          *           arguments or is part of a single argument.
          *
          * @return an {@code Optional<String>} of the command line
          *         of the process
          */
         public Optional<String> commandLine();


ProcessHandle.java:252: in arguments() method - @apiNote is a better fit 
for the note

ProcessHandleImpl_macosx.c:276:   - indentation +4


2) ProcessHandleImpl_macosx.c:192:
     if (errno != EINVAL) ...  There was previously this test,  I'm 
concerned that if the pid is invalid,
     it will now throw a RuntimeException instead of returning -1.
     I recall a discussion from May that recommended testing for EINVAL.
     The sysctl in getCmdlineAndUserInfo also does not throw if errno != 
EINVAL, so the usage
     is not consistent (probably my coding) but needs investigation.

3) ProcessHandleImpl_solaris.c can do without the includes:
#include "jni_util.h"
#include "java_lang_ProcessHandleImpl.h"
#include "java_lang_ProcessHandleImpl_Info.h"

#include <stdio.h>

4) Ditto ProcessHandleImpl_aix.c

5) ProcessHandleImpl_unix.c: 618:  typo  "fuctions" -> "functions"

6) ProcessHandleImpl_unix.c:463: Would it be worthwhile to put 
sysconf(_SC_GETPW_R_SIZE_MAX) in a static?

7) OnExitTest.java: exits without an error, just output in the log; it 
would escape attention.
     The code respects the timeout setting.
     Suggest removing the 'return' @133; the test will produce some 
errors and when debugging
     the note will be in the log.

8) ProcessHandleImpl_unix.h:58 - update the comment to include 0 as a 
valid parent pid.

Looks pretty good,
Thanks, Roger



[1] http://openjdk.java.net/jeps/8068562 - JEP draft: javadoc tags to 
distinguish API, implementation, specification, and notes

On 8/5/2015 3:56 PM, Volker Simonis wrote:
> Hi,
>
> so here's the webrev which implements the new Info.commandLine()
> method (I chose 'commandLine() ' instead of 'cmdline()' or
> 'commandline()' because the other getters are camel case as well):
>
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8131168.v4/
>
>  From the JavaDoc of the new method:
>
> * If {@code command()} and  {@code arguments()} return non-null
> * optionals, this is simply a convenience method which concatenates
> * the values of the two functions. Otherwise it will return a
> * best-effort, platform dependent representation of the command line.
>
> This didn't change anything on MacOS X where there is no additional
> effort to get the command line.
>
> On Solaris and AIX, Info.commandLine() will always return the contents
> of psinfo.pr_psargs because there's no other method to get the exact
> arguments (i.e. Info.arguments() always returns NULL. I could
> therefore remove the extra handling of AIX/Solaris in the InfoTest
> from my initial change.
>
> On Linux, things are a little more complicated:
>
> - the initial implementation for Linux used arg[0] as 'command' if
> /proc/pid/exe wasn't readable. This was true for all the processes we
> don't own (except if we are running as root). But this information may
> be incomplete, because arg[0] may only contain the command without its
> full path. One example is 'sendmail' for which Info.command() reported
> "sendmail: accepting connections" but Info.arguments() was empty. This
> is wrong, because 'sendmail' changes its argv[] array. I have
> therefore disabled this behavior now that we have the 'commandLine()'
> method.
>
> - /proc/pid/cmdline is limited to PAGE_SIZE (normally 4096) characters
> on Linux. So strictly speaking, this isn't 'exact' information as well
> (there are plenty of complains that especially for Java programs this
> is not enough) and should go to 'commandLine()' instead to 'arguments'
> if /proc/pid/cmdline is truncated. I do check for this now.
>
> - the information in /proc/pid/cmdline can also be changed to
> something other than the original arguments if a program changes
> argv[] (which is not forbidden) but there's probably not much we can
> do to detect this. I've added a corresponding @implNote comment to
> JavaDoc of Info.arguments().
>
> - the initial implementation did not check for incomplete reads of
> /proc/pid/cmdline. This may be a problem on systems with PAGE_SIZE >
> 4096 (on Linux/ppc64 a page size of 65536 is not unusual). I'm now
> always reading the complete contents of /proc/pid/cmdline.
>
> - as far as I understand the current implementation, 'arguments()'
> returns the arguments array WITHOUT 'arg[0]' (which is the program
> name) but may
> be we should specify that more clearly in the JavaDoc of 'arguments()'.
>
> That's it. Hope you like it :)
>
> Regards,
> Volker
>
> On Fri, Jul 31, 2015 at 11:02 PM, Stuart Marks<stuart.marks at oracle.com>  wrote:
>> Hi Roger, Volker,
>>
>> Glad to see you guys are receptive to this and that it can move forward. Let
>> me know if you'd like me to help out, for example with reviews or something.
>>
>> s'marks
>>
>>
>> On 7/31/15 9:55 AM, Roger Riggs wrote:
>>> Hi Volker,
>>>
>>> I agree that adding an Info.commandline() method would be a good way
>>> to make the command line available and be able to describe that it is
>>> OS dependent and may be truncated.
>>> And having it assemble the command and arguments when they are available
>>> makes
>>> sense.
>>> As an API addition it will need a clear spec and I can run it through CCC
>>> so it
>>> gets
>>> another review and compatibility tests.
>>>
>>> Thanks, Roger
>>>
>>>
>>>
>>>
>>> On 7/31/2015 5:03 AM, Volker Simonis wrote:
>>>> On Fri, Jul 31, 2015 at 2:51 AM, Stuart Marks<stuart.marks at oracle.com>
>>>> wrote:
>>>>> On 7/29/15 11:36 AM, Volker Simonis wrote:
>>>>>>> !! ProcessHandleImpl_unix: 709-738:  I still disagree with returning
>>>>>>> truncated or incomplete
>>>>>>> values for the executable or arguments.
>>>>>>> Can anyone be a tie-breaker  (with a good rational and suggestion for
>>>>>>> how
>>>>>>> an
>>>>>>> application can use the data).
>>>>>> As I wrote, I agree to hear other opinions here.
>>>>>>
>>>>>> All I want to say that this truncated data is actually what 'ps' is
>>>>>> reporting on Solaris since decades and people seem to be happy with
>>>>>> it. As use case I can imagine logging or monitoring (something like
>>>>>> 'top' in Java) where this data will be just good enough.
>>>>>>
>>>>>> We could specially mark possibly incomplete data by extending the Info
>>>>>> object with functions like commandIsExact() or argumentsIsPrecise().
>>>>>> But notice that we can not statically preset these values per
>>>>>> platform. For example on Solaris, the 'command()' would return a
>>>>>> precise value for processes with the same uid like the VM but only
>>>>>> inaccurate values for all other processes. The "arguments()" would be
>>>>>> always inaccurate on Solaris/AIX.
>>>>> It seems like there are cases where either exact or only approximate
>>>>> information is available. And as you observed, you might get one or the
>>>>> other on the same platform, depending on the UID. It also might depend
>>>>> on
>>>>> process state; I believe that some information becomes inaccessible when
>>>>> the
>>>>> process enters the zombie state.
>>>>>
>>>>> I don't think we should simply ignore one case or the other, but I also
>>>>> don't think we should try to cram the different information into the
>>>>> same
>>>>> API.
>>>>>
>>>>> The current ProcessHandle.Info api has
>>>>>
>>>>>       Optional<String> command()
>>>>>       Optional<String[]> arguments()
>>>>>
>>>>> It sounds to me like Roger wants these to contain only exact
>>>>> information.
>>>>> That seems reasonable, and this probably needs to be specified more
>>>>> clearly
>>>>> to contrast with what's below.
>>>>>
>>>>> On Solaris, the psinfo_t struct has char pr_psargs[PRARGSZ] which is a
>>>>> single string which appears to be the concatenation of the arguments
>>>>> (maybe
>>>>> including the command name). It's also truncated to fit PRARGSZ. It
>>>>> doesn't
>>>>> make sense to me to try to return this as a String[], as the zeroth
>>>>> element
>>>>> of that array, and certainly not parsed out into "words". So maybe
>>>>> instead
>>>>> we should have a different API that returns an imprecise command line as
>>>>> a
>>>>> single string:
>>>>>
>>>>>       Optional<String> cmdline()
>>>>>
>>>>> (Naming bikeshed TBS). The semantics would be that this is the process'
>>>>> command and arguments concatenated into a single string (thus
>>>>> potentially
>>>>> losing argument boundaries) and also possibly truncated based on
>>>>> platform
>>>>> (COUGHsolarisCOUGH) limitations. It's certainly useful for printing out
>>>>> in a
>>>>> ps, top, or Activity Monitor style application, for informational
>>>>> purposes.
>>>>>
>>>>> If this were implemented, then on Solaris, command() and arguments()
>>>>> would
>>>>> always return empty optionals.
>>>>>
>>>>> I'm not sure what this should be if the exact information is available.
>>>>> It
>>>>> would be inconvenient if something that just wanted to print out an
>>>>> approximate command line had to check several different APIs to get the
>>>>> information. Maybe cmdline() could assemble the information from exact
>>>>> data
>>>>> if it's is available, by concatenating the Strings from command() and
>>>>> arguments(), as a convenience to the caller. But I could go either way
>>>>> on
>>>>> this.
>>>>>
>>>>> Not sure this counts as a tie-breaker, but it might be a reasonable way
>>>>> forward.
>>>>>
>>>>> s'marks
>>>>>
>>>> Hi Stuart,
>>>>
>>>> thanks a lot for your comments - I like your proposal. For me this
>>>> sounds like a good compromise.
>>>>
>>>> @Roger: should I go and add a new field commandLine and the
>>>> corresponding getter to the Info class? As Stuart proposed, the getter
>>>> could check if 'command' and 'arguments' are available and assemble
>>>> the command line from them. Otherwise it could use the content of the
>>>> commandLine field if that is available.
>>>>
>>>> Regards,
>>>> Volker




More information about the core-libs-dev mailing list