Juan Hernandez has posted comments on this change.
Change subject: bootstrap: detach OVirtUpgrader from VdsInstaller into
OVirtNodeUpgrade
......................................................................
Patch Set 10: (6 inline comments)
....................................................
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:
In the rest of the code of the project switch statements are formatted as
follows:
switch(severity) {
case INFO:
...
break;
...
}
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/OVirtNodeUpgrade.java
Line 79: */
Line 80: @Override
Line 81: public void finalize() {
Line 82: close();
Line 83: }
It is not used, as you said, and it is a bad practice that people (like me)
will continue to point out.
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() {
If the user can't guarantee that this code will always run (using a finally
block somewhere, for example), then we have a bug, regardless of having or not
having a finalizer. We can't rely on finalizes for clean-up of resources, that
is a bug.
Line 89: stop();
Line 90: if (_dialog != null) {
Line 91: _dialog.disconnect();
Line 92: _dialog = null;
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/EngineSSHDialog.java
Line 70:
Line 71: entry = (KeyStore.PrivateKeyEntry)ks.getEntry(
Line 72: alias,
Line 73: new KeyStore.PasswordProtection(
Line 74: password.toCharArray()
Having the password stored in a string is a error, but not in this patch. But
this code is making it worse because it is copying the content of the string to
two arrays in memory, once each time you call the toCharArray method. Those two
copies will stay in memory at least till they are garbage collected, and even
after that as the garbage collection doesn't clean memory.
What I am suggesting you is to limit the number of times that you copy the
password, for example:
char[] passwordCharacters = password.toCharArray();
Then you can pass this local variable to the KeyStore methods instead of
creating a new copy.
When you are done with the password you can clean it from memory explicitly:
Arrays.fill(passwordCharacters, '\0');
Being able to do this is the reason that the parameter to the KeyStore methods
is a char[] and not a String.
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: }
And it is still a bad practice that we should stop using.
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: );
Using StringBuilder (not StringBuffer) or plain string concatenation would
certainly be faster. But what I was suggesting is to avoid the log statement if
the debug level is not enabled:
if (log.isDebugEnabled()) {
log.debug(...);
}
That avoids the expensive evaluation of the arguments if the debug level is not
enabled.
Line 231:
Line 232: try {
Line 233: if (_client != null) {
Line 234: throw new IOException("Already connected");
--
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
