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.
