Alon Bar-Lev has posted comments on this change.
Change subject: bootstrap: detach OVirtUpgrader from VdsInstaller into
OVirtNodeUpgrade
......................................................................
Patch Set 10: (14 inline comments)
Thank you for review!
> Don't use the _ and s_ prefixes.
I tried and failed. The _, s_ convention helps in avoiding variable shadowing,
helps in review code hacks, as it clears what is a member/private/static, it
helps to distinguish between public exported interface and internal methods,
and it helps in coding as it is clear what the scoping effect of each statement.
This convention is not exposed outside the class, so it does not effect anyone
who is not maintaining the code.
Keep in mind that people are using git, gerrit and other editors to review and
manage the code, thus the famous syntax highlighting is not available, and
people still need to be effective, code convention ease this without any extra
cost of tool dependency.
I think that you can learn and lavarage this as well if you open your mind for
these kind of changes.
Thank you.
....................................................
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:
I always put default as I don't know if the enum will be changed in future.
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() {
This was the same in previous code. are we sure we want to change this now?
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 {
There is a thread within class, I need runnable interface.
What do you suggest?
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);
Done
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: }
We discussed this. This will take effect if developer forgets. I don't punish
my-self or others for errors.
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() {
The user of the class.
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.
Done
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.
Done
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());
I don't understand.
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()
Why? once twice 1000 times?
There is no secured buffer in java, and java does clear buffers after
allocation.
Please suggest what you expect.
BTW: This is as-is implementation of the previous code.
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: }
Same answer as previous, and in past discussion.
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: );
Yes.
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: }
LinkedList is much lighter, no? I won't pass size... I can just replace
LinkedList with ArrayList as constructor handles all...
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;
Why? it belongs to the org.junit group. I prefer group by package and not by
semantic.
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 <alo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
