Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited

[Michael McMahon]

Ulf Zibis wrote:
> Am 16.04.2011 12:48, schrieb Alan Bateman:
>> Michael McMahon wrote:
>>> I have incorporated much of Ulf's approach into this version.
>>>
>>> I agree with Alan about the spec. We could find other variables in 
>>> the future that need
>>> to be set, but can have alternative values other than the default. I 
>>> think this approach
>>> is the most flexible.
>>>
>>> http://cr.openjdk.java.net/~michaelm/7034570/webrev.4/
>> This looks much better - thanks Ulf for the improved loop and 
>> changing it to use nameComparator. It will be good to get this one 
>> fixed.
>>
>
> Thanks for the partial acceptance.
>
> I think, the comment in lines 295..297 more belongs to the code (lines 
> 312..315 + 318..320 ) rather than to the constant SYSTEMROOT. Why 
> defining it as class member, as it is only locally referred?
>
It makes no difference to the generated code. And imo, there is more 
space for the comment there.
> I still don't see the advantage, having the SYSTEMROOT insertion code 
> at 2 call sites. One would be shorter and much clearer as shown in my 
> code. Then we too could save addToEnvIfSet(..) and the call to it.
>
Shorter doesn't always mean clearer. I think this way is clearer.
> Additionally: Why instantiating, initializing and processing a fat 
> j.u.List object, if a simple array would fulfil all needs?
>
I don't see a compelling reason to change it from a List (which is what 
it was). I think any additional overhead
is inconsequential in the context of creating an OS process. Or if it 
is, then we should be looking at
our List implementations.
> You have removed the 'Ensure double NUL termination' code. I guess, 
> you have approved, that String->CString already handles this, right?
>
Actually no. That shouldn't have been omitted. Thanks for finding that!

- Michael.