Review request for 5049299

Fri May 22 19:30:01 UTC 2009

Hi Michael,

I am very happy to see this being worked on;
I would have done something like this
already if I could fork() myself.

This code is very tricky and has had many subtle bugs over the years.

---

Any way we could add Linux support to this,
via some flavor of low-level clone+exec?
Let me restate that more strongly -
we would really really want to solve the
memory problem for Linux as well - but how?

---

Remove space before (

return System.getProperty ("java.home");

Our C coding style is idiosyncratic,
but there also remove spaces for
function calls.

---

Isn't spawn only supported as of Solaris 10?
I don't see anything in the change for
older versions of Solaris.

---

The standard way of detecting presence of a function
at runtime is
dlsym(RTLD_DEFAULT, "function_name") != NULL

Wait! libc also has posix_spawn and friends!
On many systems we should be able to find posix_spawn.
Perhaps in most common special cases we can dispense with processhelper
and spawn the target executable directly?

---

processhelper should probably be named something more obviously java-related
e.g. javaprocesshelper.
Can we try to not install it in BINDIR, since it is not intended to be run
by users,
but hide it in some private subdir?

---

Please add include guards to

processutil_md.h

even though you can get away with not having them.

---

 296  * Reads nbyte bytes from file descriptor fd into buf,

Change comma to period.

---

processutil_md.h

can be made smaller.
Utilities such as

isAsciiDigit

can be static functions called from processutil_md.c

---

  41  * Utility used by j.l.ProcessBuilder (via UNIXProcess) to launch

In source code, let's splurge and write out java.lang instead of j.l.

---
  47  * argv[1] working directory for command

Add "or the empty string if none provided"

---

You appear to be both reading and writing to single fd pipefd.
I don't think bidirectional pipes are standard Unix.
I'd prefer to see separate read and write pipes.

---

  65  * - Msg type (4 bytes)
  66  *      1 = chdir error (detail in Field 1)
  67  *      2 = exec error (detail in Field 1)
  68  *      3 = target terminated (exit code in Field 1)

Please use constants

#define CHDIR_ERROR 1
etc...

---

It appears that Msg type 3 is never used with reply.

---

 100     FILE *f;

Unused?

---
  92     int child;

Unused?

---

  97     char name [256];

Unused?

---

 164     } else {
 165         /* empty environment */
 166         env = &nullptr;
 167     }

It feels wrong to have to special-case the
empty environment.

---

  26 #undef  _LARGEFILE64_SOURCE
  27 #define _LARGEFILE64_SOURCE 1

There's probably a reason why we want
_LARGEFILE64_SOURCE
defined in the new source files.

---

Martin

---On Fri, May 22, 2009 at 03:05, Michael McMahon
<Michael.McMahon at sun.com>wrote:

> Hi,
>
> I have just posted a webrev for 5049299: (process) Use posix_spawn, not
> fork, on S10 to avoid swap exhaustion.
>
> webrev location: http://cr.openjdk.java.net/~michaelm/5049299/webrev.00/<http://cr.openjdk.java.net/%7Emichaelm/5049299/webrev.00/>
>
> **I'd like to give an outline of the change here, to make reviewing the
> webrev a bit easier.
> Basically, while posix_spawn() is a fairly elaborate and complicated system
> call, it can't quite do
> everything that the old fork()/exec() combination can do, so the only
> feasible way to do this, is to
> use posix_spawn to launch a new helper executable "processhelper", which in
> turn execs the
> target (after cleaning up file-descriptors etc.) The end result is the same
> as before, a child process
> linked to the parent in the same way, but it avoids the problem of
> duplicating the parent (VM) process
> address space temporarily, before launching the target command.
>
> In the original implementation, the native code for
> UNIXProcess.forkAndExec() was the same
> for both Linux and Solaris.  Now, we have split it, so that Linux retains
> the original implementation
> but for Solaris, the native method is renamed spawn() where the
> implementation is partly in this function,
> but also partly in the new processhelper binary, which is spawned by the
> spawn() method.
>
> A number of utility functions, which were originally in UNIXProcess_md.c,
> are also needed by the
> processhelper binary, have been moved into new source files
> (processutil_md.[ch]).
> Because the functions were originally static in UNIXProcess_md.c, a prefix
> is added to their names (jlup_)
> (from initials of java.lang.UNIXProcess), to avoid any conflict in global
> scope. This applies to the Linux
> version as well as the Solaris version. So, this looks like new code, but
> the body of these functions
> are not changed from before.
>
> I'm proposing not to add any unit/regression tests for this change, since
> the point of it
> is kind of self-evident from the source code (ie. stop using fork() and use
> posix_spawn() instead),
> and there shouldn't be any behaviour change other than memory usage.
> This area of the platform seems to be well covered by existing
> regression/unit tests.
>
> Hope this helps,
> Thanks,
> Michael.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20090522/0d99bc21/attachment.html>