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

[Roger Riggs]

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.

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.
The time units on the starttime and cputime need should appear in the 
function header
without having to refer to the declaration.

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

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

691:  Typo: Solris
730: separeted

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.

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.

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


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

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

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.

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

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

That is  first pass on comments.

Thanks, Roger



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