[Pki-devel] [PATCH] 90 Fixes for comments on patches 87 and 89

[Endi Sukma Dewata]

On 4/10/2014 9:19 AM, Abhishek Koneru wrote:
> Please review the patch with fixes for comments given by Ade and Endi.
> All the solutions were discussed on IRC. All comments are addressed
> except comment 10 in Endi's comments and one comment from Ade to add a
> cli option to archive symmetric key from its binary string.
>
> Also attaching patches 87, 89. Please apply them in this order 87, 89,
> 90
>
> --Abhishek

Some more comments:

1. In KeyGenerateCLI there's a typo in the description for --usages 
parameter: seperated.

2. In KeyGenerateCLI the description for --key-size parameter says "This 
is required for AES and RC2" only but it looks like the parameter is now 
required for RC4 too.

3. In KeyTemplate.java it's not necessary to call super().

4. In KeyTemplate.java the attribute name and variable name for "ID" 
should be lower case "id". In general attribute/variable names are 
internal, so they should follow Java convention (i.e. lower case "id"). 
Upper case attribute names are reserved for constants. Outputs that can 
be seen by users should use the proper English capitalization (i.e. 
upper case "ID").

5. Please run source formatter on SymKeyGenerationRequest.java.

6. There's an extra semicolon in KeyClient.java:

   String[] list = usages.split(",");
   String invalidUsages = null;
   ;
   for (int i = 0; i < list.length; i++) {
       ...

7. The above code can be simplified into:

   List<String> invalidUsages = new ArrayList<String>();
   String (String usage : usages.split(",")) { // use for-each
       if (!validUsages.contains(usage))
           invalidUsages.add(usage);
   }

In general it's better to use a collection to construct a list rather 
than doing string concatenation. It's also possible to throw the 
exception immediately when an invalid usage is found. It's not really 
necessary to list all invalid usages.

8. I think KeyClient.generateSymmetricKey() should take an array or List 
of string usages instead of a single string containing concatenated 
usages. In general an API should take parameters in the parsed format 
that can be used immediately without further processing. In this case 
the KeyGenerateCLI and the DRMTest should construct a list of string 
usages first, then pass it to generateSymmetricKey().

9. The KeyTemplateFindCLI is still hardcoding the list of templates. It 
should get the list of files from the templates folder and generate the 
template names from the file names.

How about renaming the /usr/share/pki/kra/templates folder into 
/usr/share/pki/key/templates and renaming the XML files to match the 
template name?

10. The KeyTemplateShowCLI is still partially hardcoding the templates. 
I think the XML file path can be generated from the specified template 
ID parameter. Then the class name can be obtained from the ClassName 
element in the XML file. And the "message" can be obtained from another 
attribute stored in the template (e.g. description).

11. In KeyTemplateShowCLI the code is checking for the length of the 
filename parameter. As mentioned in the previous email this check is not 
necessary and could be misleading.

   if ((writeToFile != null) && (writeToFile.trim().length() != 0)) {

Also the above line contains some redundant parentheses.

12. In KeyTemplateShowCLI the printRequestTemplate() and 
readFromTemplateFile() probably can be moved to ResourceMessage and be 
called marshal() and unmarshal() as variations of the existing methods.

-- 
Endi S. Dewata