Request for Review: 5049299: (process) Use posix_spawn, not fork, on S10 to avoid swap exhaustion

[Alan Bateman]

On 25/07/2013 09:17, Rob McKenna wrote:
> Thanks a lot Erik,
>
> I've added the dependency to the makefile here:
>
> http://cr.openjdk.java.net/~robm/5049299/webrev.05/ 
> <http://cr.openjdk.java.net/%7Erobm/5049299/webrev.05/>
>
> Is it ok between the ifeq block?
>
> Alan,
>
> I've altered the launchMechanism code to use valueOf (and lower case 
> names) - much cleaner, thanks for that. I've also limited each 
> platform to supported mechanisms only. With the new layout I don't 
> believe a separate test is needed here. (the default is now 
> posix_spawn/vfork and Basic.java has been altered to run with fork too)
>
>     -Rob
Thanks Rob, I've taken another pass over the latest webrev and I think 
the finish line is close.

The launchMechanism determination is much cleaner now (thanks). For 
consistency the enum values should probably be in uppercase and so this 
means you'll need to uppercase the property value to use valueOf.

It would be nice if launchMechanism were final too (which you do by 
having the privileged action be of type LaunchMechanism rather than Void).

The comment in UNIXProcess.java.bsd references vfork/exec which I assume 
is copied from the Linux version and should be removed.

Did you consider having forkAndExec take the helper as a parameter? Just 
wonder if this would be cleaner than having to use JNI to grab the field 
value.

It's usually best to use sprintnf rather than sprintf (in spawnChild) to 
avoid any concerns (or static analysis tools) that wonder about buffer 
overflow.

Style nit in the C code is that many places have spurious space between 
the function name and the parentheses.

A pre-existing bug (only noticed because it has moved from 
UNIXProcess_md.c to childproc.c) is that close is not restartable.

The comment in in makefiles/CompileLauncher.gmk references the old 
build, is that needed?

So overall I don't see any issues, it would be really good to stress 
this to make sure that we aren't leaking file descriptors. I don't know 
if we have an existing test that would help with that.

One final comment is that the new files seems to have the pure GPL 
comment, I assume you will add the GPL+ Classpath Exception comment 
before you push this.

-Alan.