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.
A minor comment is that -Djdk.lang.Process.allowAmbigousCommands=false
will not do what is intended (the value of the property is not examined).
The test needs a copyright header but otherwise looks comprehensive. 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.
Minor comments on the test is that the creation of the commands could
use try-with-resources.
-Alan.
