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

[Volker Simonis]

Hi Roger,

thanks a lot for the review. Please find my comments inline and the
new webrev under:

http://cr.openjdk.java.net/~simonis/webrevs/2015/8131168.v3/

On Mon, Jul 27, 2015 at 10:26 PM, Roger Riggs <Roger.Riggs at oracle.com> wrote:
> Hi Volker,
>
> Thanks for the refactoring and the AIX implementation.
>
> We still need a tie-breaker with respect to the issue of
> truncated/incomplete
> argument and executable values.
>

Yes, I agree. Please see my other comments further down.

> On 7/24/2015 9:30 AM, Volker Simonis wrote:
>
> Hi,
>
> so here comes the new webrev which cleanly applies to the current
> jdk9-dev/jdk repo:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2015/8131168.v2/
>
>
> Comments:
>
> os_getParentPid() - the function name doesn't reflect the additional values
> returned.

Yes, but the initial name getStatInfo() wasn't perfect either because
for example on MacOS X there is no stat-file at all.

I've changed it to os_getParentPidAndTimings() for now but I'm open
for better suggestions :)

> The time units on the starttime and cputime need should appear in the
> function header
> without having to refer to the declaration.
>

Done.
I've also reordered the function declarations in
ProcessHandleImpl_unix.h to group the corresponding os_ and unix_
functions together.

> os_getParentPid()  - Within this function can the code that accesses the
> sysctl info be kept together.
>    Perhaps move the rusage if/block before the if (sysctl) {...}
>    Or just move the start time extraction to just before the return pid..
>

It was this way in the initial code already, but I've changed it as requested.

> ProcessHandleImpl_unix.c: 138 and 140 - The macros should not include the
> trailing semicolon ";"
> (causes a unreachable compilation warning on Solaris 10)
>
> ProcessHandlmpl_unix.c: 401: extra semicolon  ";;"
> (causes a warning about declarations after statements on Solaris 10).
>

Ah, good catch! I saw these warnings but I thought they are related to
my local compiler.
Fixed.

> 691:  Typo: Solris
> 730: separeted
>

Fixed.

> ProcessHandleImpl_unix:c: 358 - Info_info0 - it appears that the psinfo file
> is read twice.
> Once by unix_getUid and again by unix_getCmdLineInfo
> One aspect of the original code  was to avoid opening and reading /proc
> files more than once/necessary.
>

OK, I removed os/unix_getUid() and merged the call of
unix_getUserInfo() into os/unix_getCmdlineInfo() which I've renamed
into os/unix_getCmdlineAndUserInfo()

> ProcessHandleImpl_unix.c: 691-700 - There is no point in doing a file lookup
> that is
> known to fail.  This should be #ifdef/#ifndef'd to apply only to the
> appropriate OS.
>

Fixed.

> !! 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.

Maybe we should cast a vote among the initial "8077350: JEP 102
Process API Updates Implementation" reviewers?

>
> InfoTest.java: 139:  Is this the best that can be done for checking the
> times on AIX?
>

Yes, because /proc/<pid>/status is only updated once a second on AIX
(see: http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2015-July/002273.html).
I've slightly updated the comment.

> InfoTest.java:285 - mentions /proc/pid/status ;  should that be
> /proc/pid/psinfo?
>

Yes, you're right. I've updated both occurences.

> ProcessHandleImpl_linux.c: 63: getUID and getCmdlineInfo both format the
> /proc file
> name and do a lookup of the file.  The original version avoided doing the
> lookup twice
> in the same native call.    Perhaps there is a refactoring that puts the
> extraction of
> the UID in with getting the command line.
> This might address the repeated reading of psinfo in
> ProcessHandleImpl_unix:358 as well.
>

Done. See comments for ProcessHandleImpl_unix:358 above.

> ProcessHandleImpl_unix.h is a new file and only needs a 2015 copyright date.
>

Done. Also for ProcessHandleImpl_aix.c and ProcessHandleImpl_linux.c

> Somewhere the unixs for the start and cputimes needs to be documented
> at the declaration.
>

Done.

> That is  first pass on comments.
>
> Thanks, Roger
>

I hope the dispute about the truncated/incomplete Info fields remains
the last open point for now :)

Regards,
Volker

>
>
>
> The good thing beforehand - although I added the AIX-port and a big
> 50-line comment the new version is still 60 lines shorter :)
>   10 files changed, 829 insertions(+), 999 deletions(-)
>
> The main idea behind the new code layout is as follows (see the
> comment in ProcessHandleImpl_unix.c for more details):
>
> The currently supported Unix variants are Solaris, Linux, MaxOS X
> and AIX. The various similarities and differences between these
> systems make it hard to find a clear boundary between platform
> specific and shared code.
>
> In order to ease code sharing between the platforms while still
> keeping the code as clean as possible (i.e. free of preprocessor
> macros) we use the following source code layout (remember that
> ProcessHandleImpl_unix.c will be compiled on EVERY Unix platform
> while ProcessHandleImpl_<os>.c will be only compiled on the
> specific OS):
>
>  - all the JNI wrappers for the ProcessHandleImpl functions go
>    into ProcessHandleImpl_unix.c file
>
>  - if their implementation is common on ALL the supported Unix
>    platforms it goes right into the JNI wrappers
>
>  - if the whole function or substantial parts of it are platform
>    dependent, the implementation goes into os_<function_name>
>    functions in ProcessHandleImpl_<os>.c
>
>  - if at least two platforms implement an os_<function_name>
>    function in the same way, this implementation is factored out
>    into unix_<function_name>, placed into
>    ProcessHandleImpl_unix.c and called from the corresponding
>    os_<function_name> function.
>
>  - For convenience, all the os_ and unix_ functions are declared
>    in ProcessHandleImpl_unix.h which is included into every
>    ProcessHandleImpl_<os>.c file.
>
> So ProcessHandleImpl_unix.c only contains code which is shared by at
> least two platforms. Code which is specific to one single platform
> goes into the corresponding ProcessHandleImpl_<os>.c file (and this
> changes adds ProcessHandleImpl_aix.c for AIX-specific code and
> ProcessHandleImpl_linux.c for Linux-specific code which was in
> ProcessHandleImpl_unix.c previously).
>
> I tried to unify similar functions (e.g.
> Java_java_lang_ProcessHandleImpl_00024Info_initIDs,
> Java_java_lang_ProcessHandleImpl_isAlive0 or fillArgArray) into a
> single instance in ProcessHandleImpl_unix.c.
>
> I renamed and unified getStatInfo() into
> os_getParentPid()/unix_getParentPid() with three implemantions for
> MacOSX, Linux and Solaris/AIX.
>
> I've factored out Java_java_lang_ProcessHandleImpl_getProcessPids0()
> into os_getChildren()/unix_getChildren() with two implemantions - one
> for MacOSX and one for Linux/Solaris/AIX.
>
> I've tested the new implementation on MacOSX, Linux (x86_64 and ppc64)
> , Solaris 10/11 (x86_64 and Sparc) and AIX.
>
> The final point, which was already mentioned in the first review is
> the fact that we can not easily get accurate information fort he
> command line and arguments on AIX. I'd prefer to stay with the current
> solution which gives "best effort" answers for these attributes but if
> everybody’s opinion is that such information is useless I can also
> omit it altogether. I haven’t looked at the other platforms which we
> have to support (e.g. HPUX, AS/400) but the situation may be similar
> there.
>
> Regards,
> Volker
>
>
>
>