Alon Bar-Lev has posted comments on this change.

Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................


Patch Set 4: (11 inline comments)

....................................................
File backend/manager/modules/utils/pom.xml
Line 156:         <excludes>
Done

....................................................
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);
Didn't touch this.

Line 77:     private boolean _doConnect(String server, Credentials creds) {
private functions should be distinct clearly.

Line 146:                             log.error(e);
Well, as this is truly a debug mean and nothing can be done, I don't think any 
message will help...

But added.

Line 178:                 String tmp_fingerprint = 
OpenSSHUtils.getKeyFingerprintString(serverKey);
Done

Line 217:     public void finalize() throws Exception {
Both of you, we should guard developers from them-selves.

If one forget for some reason to call disconnect, we have the possibility to 
clean up using the gc.

It is far from being bad practice, and it may save us from leaking or keeping 
resources busy.

Line 307:                             log.error(e);
Done

Line 329:             public void finallize() {
Again, same as above.

And thanks for noting the spelling, I forgot this one.

Line 340:                         log.error(e);
Done

....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/SSHClient.java
Line 77:             throw new IllegalArgumentException("File name should not 
contain \"'\"");
This class is infrastructure only, the log is produce from the application 
(user) side. I added debug which was a valid request, but low level classes 
should not write into logs.

Line 474:                         log.debug(e);
No, the application should handle this, we will appear in the stack dump.

For debugging purposes I added this one as well, which is unnecessary, but 
exists.

--
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

Reply via email to