Review request: JDK-8012453 (process) Runtime.exec(String) fails if command contains spaces [win]

[Alexey Utkin]

Alan,

Thanks for your comments.
I did the changes in code.
Please, read my answers below.

Here is fixed version:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8012453/webrev.02/

Regards,
-uta


On 24.04.2013 18:23, Alan Bateman wrote:
> On 24/04/2013 13:58, Alexey Utkin wrote:
>>
>> I changed in the ProcessBuilder class to restore the compatibility 
>> with Java documentation. In accordance with spec, the 
>> IllegalArgumentException exception could not be thrown from the start 
>> method. I made it a cause for declared IOException.
> This part looks good to me.
>
>
>>
>> Thanks for your attention. The test was extended to cover the case.
>>
>> New version:
>> http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-8012453/webrev.01/
>>
>> Lines 381-382 in ProcessImpl.java file are responsible for the second 
>> call to Security Manager.
>> The call with [".\Program Files\doNot.cmd" arg] param does not pass 
>> the second Security Manager verification in the ExecCommand.java test.
> Overall I think this looks okay.
>
> In getTokensFromCommand then I guess I would have used a regex rather 
> than legacy StringTokenizer (I realize Runtime is specified to use 
> StringTokenizer for the initial split). I think I agree with Martin on 
> wondering why LinkedList is used but it's not an issue as this is the 
> fallback. Minor nit is that the brace on L161 can be moved to the end 
> of the previous line.
Done.

> A minor comment is that -Djdk.lang.Process.allowAmbigousCommands=false 
> will not do what is intended (the value of the property is not examined).
Done.

>
> The test needs a copyright header but otherwise looks comprehensive. 
Done.

> One thing I see is that it doesn't read from the child's output/error 
> streams so if there is a problem then it will be hard to diagnose from 
> the logs.
The diagnostic includes logging of the command line  in problem. It 
should not be a problem to localize issue.

> Minor comments on the test is that the creation of the commands could 
> use try-with-resources.
Done