RFR: 8000975: (process) Merge UNIXProcess.java.bsd & UNIXProcess.java.linux (& .solaris & .aix)

[Peter Levart]

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.Bateman at oracle.com <mailto:Alan.Bateman at 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.
>>
>>
>