[Pki-devel] [PATCH] 548 Updated CRMFPopClient parameter handling.

[Endi Sukma Dewata]

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