Hi Max,
Please see inline.
On 01/19/2017 05:15 PM, Weijun Wang wrote:
On 01/19/2017 09:40 PM, Artem Smotrakov wrote:
Hi Max,
In general, looks okay.
Would it be better if it called redirectInput() only if the response
file exists? keytool() method might also delete the response file after
reading it. These two measures may prevent situations when the response
file is unnecessary used.
Yes, it's good to remove the file after reading it, so this means
people need to set it for every call.
On the other hand, I still think creating an empty file and call
redirectInput() on it is necessary when the response file does not
exist. This prevents the keytool() call from hanging forever if it
actually needs user input but the test has not provided any.
It looks like a test issue with particular test, I don't remember many
tests which failed because of this.
Also, I am feeling that the jarsigner-related calls are quite
complicated. I suggest we use the same method for signing and
verifying and ask the user to provide all arguments in their order. So
instead of calling
SecurityTools.jarsigner("x.jar", "alias", "...");
we just call
SecurityTools.jarsigner("... x.jar alias");
This looks better to me as well.
I am fine with current webrevs.
Artem
This is consistent with keytool(), and we can also provide 3
overloaded forms.
What do you think? :-)
Thanks
Max
What do you think?
Artem
On 01/18/2017 05:50 PM, Weijun Wang wrote:
Please review the code changes at
root: http://cr.openjdk.java.net/~weijun/8172975/root/webrev.00/
jdk: http://cr.openjdk.java.net/~weijun/8172975/webrev.00/
The fix is in root repo. This is not an elegant solution because it
uses a separate method to provide the response. This means you have to
clear the response if the next keytool call does not need it. This
also means you cannot run keytool in multiple threads.
I didn't provide the response as an extra argument because there are
already many overloaded keytool() methods, and I do not want to invent
a new method name (say, keytoolWithResponse) and implement the same
number of overloaded methods.
If you are strongly against this solution, I'll think of another way.
The jdk change includes a test for this change, as well as a trivial
fix for keytool's getYesNoReply() method. Otherwise an NPE is thrown
with the following command:
cat untrusted.cert | keytool -importcert -alias a
Thanks
Max
