Re: [Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling.
On 2/25/2015 7:32 AM, John Magne wrote:
Looks fine: ACK
Thanks. Pushed to master.
One quick minor question though...
Unless I have the order wrong we have something like this:
boolean sslECDH = Boolean.parseBoolean(cmd.getOptionValue("x",
"false"));
Then further down it appears that we do the tests to make sure the switches
provided are algorithm appropriate..
if (algorithm.equals("rsa")) {
...
...
...
+
+ if (cmd.hasOption("x")) {
+ printError("Illegal parameter for RSA: -x");
+ System.exit(1);
+ }
Does this ordering matter or are we using the statement above as a convenient way
to initialize the variable "sslECDH".
The order does not really matter since it's only parsing the option
value, not interpreting the value, and the current code is more concise.
For boolean options like sslECDH, any non-boolean value will be parsed
into "false". So if you specify:
-a rsa -x <non-boolean value>
it will generate an "Illegal parameter for RSA" message.
For integer options it's a bit different, for example:
int extractable = Integer.parseInt(cmd.getOptionValue("e", "-1"));
if (algorithm.equals("rsa")) {
if (cmd.hasOption("e")) {
printError("Illegal parameter for RSA: -e");
System.exit(1);
}
}
If you specify:
-a rsa -e <non-integer value>
it will generate a NumberFormatException instead of "Illegal parameter
for RSA". But I think either way is fine.
If we really want to see an "Illegal parameter" instead of
NumberFormatException, the code has to be written as follows:
int extractable = -1;
if (algorithm.equals("rsa")) {
if (cmd.hasOption("e")) {
printError("Illegal parameter for RSA: -e");
System.exit(1);
}
} else if (algorithm.equals("ec")) {
extractable = Integer.parseInt(cmd.getOptionValue("e", "-1"));
}
which is less concise.
I think it's better to pay more attention to how the options are
interpreted and make sure that it's done in the correct order,
especially if it's resource intensive. For example loading a file after
making sure the filename option is legal.
--
Endi S. Dewata