Juan Hernandez has posted comments on this change.
Change subject: pki: remove the need to store ssh public key
......................................................................
Patch Set 10: (15 inline comments)
....................................................
File backend/manager/conf/ca/installCA.sh
Line 91
This engine.ssh.key.txt file is needed by the engine-setup tool in order to
calculate and show to the user the SSH key fingerprint.
....................................................
File backend/manager/modules/root/pom.xml
Line 38: </dependency>
Add an empty line here.
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/pki/PKIResource.java
Line 28: */
When formated as HTML this javadoc will appear all in one line. Not very nice.
Try to use HTML tags to make it look nicer.
Line 29: public class PKIResource extends HttpServlet {
It is common practice to name classes that extend HttpServlet as
WhateverServlet. Can you rename this to PKIResourceServlet?
Line 31: private static final long serialVersionUID = 6242595311775511209L;
This serial version id was copied from the previous CAKeyServlet. This won't
break anything, but in general is better to generate a new one for each new
class.
Line 42: String outputFormat =
getServletConfig().getInitParameter("output-format");
Consider converting these two local variables into fields and moving the
initialization to the "init" method. It is also good if you check for null in
that "init" method, generate an useful log message and then throw
ServeltException to mark the servlet as unavailable.
For "resourceLocation" is probably better to use java.io.File instead of
java.lang.String.
It would be good to check in the "init" method if the value of "outputFormat"
is one of the supported ones and log/throw an explicit message.
Consider using constants for the names of the parameters.
Line 47: Certificate certificate = cf.generateCertificate(in);
Consider making these two local variables final.
Line 49: if (outputFormat.equals("X509-PEM")) {
Try to use "constant".equalsIgnoreCase(variable).
Line 54: (
These parenthesis are meaningless.
Line 65: )
My crusade against String.format: this particular string formatting is approx
600% faster if you use plain string concatenation, and 800% faster if you use
StringBuilder with the right buffer size. String.format is horribly slow.
In this particular case you can use 3 print statements and then there is no
need to create an extra string.
Line 68: else if (outputFormat.equals("SSH")) {
Try to use "constant".equalsIgnoreCase(variable).
Line 74:
getServletConfig().getInitParameter("output-alias")
Consider adding an "outputAlias" field and initializing it in the "init" method.
Line 79: throw new IllegalArgumentException("Unsupported output
format " + outputFormat);
Here you should throw a ServletException, or explicitly send an error response
code, with this unchecked exception the result is undefined.
Line 82: catch(Exception e) {
Consider naming this "exception" instead of "e".
....................................................
File packaging/fedora/setup/engine-setup.py
Line 862
This means that the user will no longer see the key fingerprint once setup is
finished. Are you doing that on purpose?
--
To view, visit http://gerrit.ovirt.org/4853
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I328baded92b2e7c5169bc87e7c19680f598389b9
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
