On 05/04/2014 12:12 PM, Peter Levart wrote:
Hi Alan, Martin,
Thanks for reviews. I have prepared another webrev based on your feedback:
http://cr.openjdk.java.net/~plevart/jdk9-dev/UNIXProcess/webrev.08/
Ah, I've forgotten to mention the most important change from webrev.07.
Original code wraps the processReaperExecutor construction into the
doPrivileged() call. I think this was not needed.
The Executors.newCachedThreadPool() does not need any special
privileges. And construction of nested class ProcessReaperThreadFactory
also didn't need any special privileges - apart from static
initialization which called the getRootThreadGroup() method. But that
method did it's own doPrivileged() wrapping in order to be able to climb
the ThreadGroup hierarchy (which requires privilege). In webrev.07 I
followed original code and wrapped the construction of a
processReaperExecutor into a doPrivileged() although in webrev.07 the
rootThreadGroup search happens as part of UNIXProcess static
initialization and is already wrapped in doPrivileged(). In webrev.08 I
removed this superfluous doPrivileged(). I think this is correct.
SecurityManagerClinit test passes.
Regards, Peter
*
*
Comments inline...
On 04/30/2014 05:17 PM, Alan Bateman wrote:
I reviewed previous rounds and you've addressed my points so I think
I'm mostly happy with this. As some point I think we should look at
Platform again as there may be an opportunity later in JDK 9 to move
it out of UNIXProcess.
One thing that I wasn't sure about is the additions to the @author
tags as we've mostly been trying not to grow these (contentious
topic, I don't have a strong opinion as I never use @author).
I have removed myself, but left all the others (merged from all 4
files). It's true that mercurial keeps track of authors and it's not
hard to find out who's responsible for what part of code in case
someone needs to ask the author about the not so obvious things, but
refactorings like this, which eliminate entire files and move their
content to other places, make this tracking harder.
A minor comment looking at the latest webrev.07 is that some of the
lines are very long. It's not a problem now but I could image future
side-by-side reviews needing a scroll
bar.DeferredCloseProcessPipeInputStream is very long too.
I split the longer lines now. The DeferredCloseProcessPipeInputStream
is admittedly a long name, but it's not public and I tried to express
the fact that it is a class that merges the functionalities of
DeferedCloseInputStream and ProcessPipeInputStream (in .aix file it
was called ProcessPipeInputStream as it was derived from it, but we
can't have two with same names in common file). I hope in further
re-factoring we can merge/consolidate these wrapper streams too and at
that time I think they could be renamed. For now I wanted to keep the
functionality as-is. But I can rename it if so desired, I just can't
think of a suitable name.
On 04/30/2014 06:25 PM, Martin Buchholz wrote:
Belated review.
Peter, thank you very much for taking on this difficult unification
task. This is great work!
Comments:
---
---
+ case SOLARIS:
+ if (osArch.equals("x86")) { osArch = "i386"; }
It's very sad that ... there's no portable way of getting the name of
libarchdir. I thought os.arch system property would suffice, but
apparently not on Solaris?
...and BSD (OS X). Is OS X missing the architecture sub-directories
because it only supports one architecture now or because it used to
employ "fat" (multi-architecture) binaries?
---
To avoid repetition, I would iniitalize with a vararg list of launch
mechanisms, of which the first one is always the default.
+ LINUX(LaunchMechanism.VFORK,
EnumSet.of(LaunchMechanism.FORK, LaunchMechanism.VFORK)),
LINUX(LaunchMechanism.VFORK, LaunchMechanism.FORK),
I've done that. It looks nicer now.
---
Never use toUpperCase without a Locale, to avoid the "Turkish i problem"
+ lm =
LaunchMechanism.valueOf(s.toUpperCase());
toUpperCase(Locale.ENGLISH)
Oops, thanks for spotting this. Fixed.
Regards, Peter
On Wed, Apr 30, 2014 at 8:17 AM, Alan Bateman
<alan.bate...@oracle.com <mailto:alan.bate...@oracle.com>> wrote:
On 25/04/2014 17:47, roger riggs wrote:
Hi Peter,
Including the test update with the updated changeset is fine.
(I think Alan had some comments on the refactoring and has
not yet had a chance to comment).
Thanks, Roger
I reviewed previous rounds and you've addressed my points so I
think I'm mostly happy with this. As some point I think we should
look at Platform again as there may be an opportunity later in
JDK 9 to move it out of UNIXProcess.
One thing that I wasn't sure about is the additions to the
@author tags as we've mostly been trying not to grow these
(contentious topic, I don't have a strong opinion as I never use
@author).
A minor comment looking at the latest webrev.07 is that some of
the lines are very long. It's not a problem now but I could image
future side-by-side reviews needing a scroll
bar.DeferredCloseProcessPipeInputStream is very long too.
-Alan.
