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
