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

[roger riggs]

Hi Peter,

Looks fine to me.

Many thanks, Roger

On 5/4/2014 6:26 AM, Peter Levart wrote:
>
> 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.
>>>
>>>
>>
>