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