Re: [Pki-devel] [PATCH] 90 Fixes for comments on patches 87 and 89
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