Juan Hernandez has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 4: (12 inline comments)
I am leaving aside the use of "_" in the names, but insisting in the use of
finalize, in logging and in concurrency issues. Hope you don't hate me by now.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/MinaInstallWrapper.java
Line 28: private static Log log =
LogFactory.getLog(MinaInstallWrapper.class);
It is good to have "final" here.
Line 146: log.error(e);
A message like "Can't close keystore file ..." could help here.
Line 178: String tmp_fingerprint =
OpenSSHUtils.getKeyFingerprintString(serverKey);
Typical variable name would be "tmpFingerprint".
Line 217: public void finalize() throws Exception {
We already discussed the use of "finalize" in patch set 1. It is ok to use it
if the "wrapperShutdown" method is explicitly called somewhere else.
Calling it "destructor" in the comment is not accurate, as a Java finalizer is
not equivalent to a C++ destructor.
Line 307: log.error(e);
A more explicit message here (in the log) could help, something like "Can't
decode output from SSH command \"" + command + "\" using UTF-8.".
Line 329: public void finallize() {
This is misspelled.
Line 340: log.error(e);
I more explicit message could help.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/SSHClient.java
Line 39: private static Log log = LogFactory.getLog(SSHClient.class);
Add "final" here.
Line 77: throw new IllegalArgumentException("File name should not
contain \"'\"");
I would like to see this in the log.
Line 474: log.debug(e);
If there is an error here it should be logged with ERROR level, DEBUG is not
enough.
A explanatory message could help: log.error("Error while sending file ...", e);
Line 502: t.join(1000); // attempt to make sure thread ended
Correct, just an attempt. See the comments in patch set 1.
Line 595: t.join(1000); // attempt to make sure thread ended
Same as in the send file method.
--
To view, visit http://gerrit.ovirt.org/6722
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I50ba60f2db364114907485da3074feb714615e0c
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@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: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@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
