Juan Hernandez has posted comments on this change.
Change subject: bootstrap: detach OVirtUpgrader from VdsInstaller into
OVirtNodeUpgrade
......................................................................
Patch Set 10: (14 inline comments)
Don't use the _ and s_ prefixes.
Some more comments inside.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallerMessages.java
Line 32: case INFO:
Line 33: logType = AuditLogType.VDS_INSTALL_IN_PROGRESS;
Line 34: log.infoFormat("Installation {0}: {1}",
_vds.gethost_name(), text);
Line 35: break;
Line 36: default:
This "default" is not needed as all the values of the Serverity enum are
covered.
Take a look at how the switch statements are formated in the rest of the code
and consider adapting to it.
Line 37: case WARNING:
Line 38: logable.setCustomId(_sequence++);
Line 39: logType = AuditLogType.VDS_INSTALL_IN_PROGRESS_WARNING;
Line 40: log.warnFormat("Installation {0}: {1}",
_vds.gethost_name(), text);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/InstallVdsCommand.java
Line 75: return result;
Line 76: }
Line 77:
Line 78: @Override
Line 79: protected void executeCommand() {
I see that there are at least 9 calls to getVds() in this method. Consider
using a local variable.
Line 80: if (
Line 81: getVds() != null &&
Line 82: isOvirtReInstallOrUpgrade()
Line 83: ) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeUpgrade.java
Line 22:
Line 23: /**
Line 24: * ovirt-node upgrade.
Line 25: */
Line 26: public class OVirtNodeUpgrade implements SSHDialog.Sink, Runnable {
Why is this a runnable? Is the user of this class expected to call its "run"
method? It looks to me that this is an internal detail of the implementation
and shouldn't be exposed.
Line 27:
Line 28: private static final int BUFFER_SIZE = 10 * 1024;
Line 29: private static final int THREAD_JOIN_TIMEOUT = 20 * 1000;
Line 30:
Line 69: _vds = vds;
Line 70: _iso = Path.Combine(Config.resolveOVirtISOsRepositoryPath(),
iso);
Line 71:
Line 72: _messages = new InstallerMessages(_vds);
Line 73: _thread = new Thread(this);
Give a name to the thread.
Line 74: _dialog = new EngineSSHDialog();
Line 75: }
Line 76:
Line 77: /**
Line 79: */
Line 80: @Override
Line 81: public void finalize() {
Line 82: close();
Line 83: }
Don't use the finalize method, do proper clean-up instead.
Line 84:
Line 85: /**
Line 86: * Free resources.
Line 87: */
Line 84:
Line 85: /**
Line 86: * Free resources.
Line 87: */
Line 88: public void close() {
Who is calling this method, other than the finalizer that migth never be called?
Line 89: stop();
Line 90: if (_dialog != null) {
Line 91: _dialog.disconnect();
Line 92: _dialog = null;
Line 93: }
Line 94: }
Line 95:
Line 96: /**
Line 97: * Main function.
This is not a function, but a method.
Line 98: * Execute the command and initiate the dialog.
Line 99: */
Line 100: public void execute() throws Exception {
Line 101: try {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/EngineSSHDialog.java
Line 39: );
Line 40: }
Line 41:
Line 42: /**
Line 43: * Get host fongerprint.
s/fongerprint/fingerprint/, althought this comment is not very useful.
Line 44: * @return fingerprint.
Line 45: */
Line 46: public String getHostFingerprint() throws IOException {
Line 47: String fingerprint =
OpenSSHUtils.getKeyFingerprintString(getHostKey());
Line 65: InputStream in = null;
Line 66: try {
Line 67: in = new FileInputStream(p12);
Line 68: KeyStore ks = KeyStore.getInstance("PKCS12");
Line 69: ks.load(in, password.toCharArray());
Having the clear password in memory once is bad ...
Line 70:
Line 71: entry = (KeyStore.PrivateKeyEntry)ks.getEntry(
Line 72: alias,
Line 73: new KeyStore.PasswordProtection(
Line 70:
Line 71: entry = (KeyStore.PrivateKeyEntry)ks.getEntry(
Line 72: alias,
Line 73: new KeyStore.PasswordProtection(
Line 74: password.toCharArray()
... but twice is even worse.
Line 75: )
Line 76: );
Line 77: }
Line 78: catch (Exception e) {
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/SSHDialog.java
Line 103: */
Line 104: @Override
Line 105: public void finalize() {
Line 106: disconnect();
Line 107: }
Don't use finalizers, do proper clean-up instead.
Line 108:
Line 109: /**
Line 110: * Get session public key.
Line 111: * @return public key or null.
Line 226: _port,
Line 227: _hardTimeout,
Line 228: _softTimeout
Line 229: )
Line 230: );
Are you aware that this very expensive use of String.format is executed every
time, not only when debug is enabled?
Line 231:
Line 232: try {
Line 233: if (_client != null) {
Line 234: throw new IOException("Already connected");
Line 289: stdinList = new LinkedList<InputStream>();
Line 290: }
Line 291: else {
Line 292: stdinList = new
LinkedList<InputStream>(Arrays.asList(initial));
Line 293: }
Any reason to use LinkedList? It would probably be better to use ArrayList and
specify the right size.
Line 294: stdinList.add(pinStdin);
Line 295:
Line 296: try {
Line 297: sink.setControl(
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ssh/SSHDialogTest.java
Line 22: import javax.naming.TimeLimitExceededException;
Line 23:
Line 24: import static org.junit.Assert.assertEquals;
Line 25: import static org.junit.Assert.assertTrue;
Line 26: import static org.junit.Assume.assumeTrue;
Static imports tend to be at the very top of classes, right after the package
specification.
Line 27: import org.junit.After;
Line 28: import org.junit.AfterClass;
Line 29: import org.junit.Before;
Line 30: import org.junit.BeforeClass;
--
To view, visit http://gerrit.ovirt.org/9174
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iff19fdb9f717d424f23bc5d4e5a8df8fce8a58bf
Gerrit-PatchSet: 10
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
