Alon Bar-Lev has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 1: (24 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/MinaInstallWrapper.java
Line 89: "_doConnect enter (%1$s, { %2$s, %3$s }, %4$d)",
Well, java... no proper resource cleanup, no preprocessor... just makes live
wonderful!
Line 199: }
For the remote case when user will not disconnect, gc may call this one.
If process terminates, connection is terminated anyway, no need to bother
Runtime for this.
Line 207: _log.debug("wrapperShutdown enter");
So left methods entry/leave.
Line 308: public void finallize() {
Just looked at java sources, as expected they are using destructors for
streams.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/OvirtSSH.java
Line 32: public class OvirtSSH {
OK.
Let it be SSHClient
Line 34: static final String COMMAND_FILE_SEND = "%1$s > '%2$s' && md5sum
-b '%2$s' | cut -d ' ' -f 1 >&2";
OK.
Line 45: PublicKey _serverKey;
OK.
I comply although I don't think it make the code any more clear.
Line 50: public static class ConstraintByteArrayOutputStream extends
ByteArrayOutputStream {
OK, although I think it does not make any more clear.
Line 100: OvirtSSH _client;
Moved to inline.
Line 140: false
It will be good when patching...
Line 149: public void finalize() {
For the remote chance we have a user that does not call disconnect and the gc
clean this up.
Line 292: * The underlined ssh library does not provide
Sorry.
Line 295: class IndexOutputStream extends FilterOutputStream {
Unlikely... but I moved to own file per your request.
Line 455: catch (IllegalArgumentException e) {
OK.
Added debug log.
Line 476: ) throws Exception {
I understand.
However, the apache ssh code throws Exception, nothing I can do.
Line 483: final MessageDigest localDigest =
MessageDigest.getInstance("MD5");
The digest is not updated in different threads. there is no problem with this
usage.
Line 485: Thread t = new Thread(
thread name - OK.
Compress - was in previous code - I did not change this. I also wondered, but
it can be removed in future.
Line 489: GZIPOutputStream zout = new
GZIPOutputStream(pout);
This is a good idea.
Line 490: byte [] b = new byte[1024];
OK.
Line 498: catch (IOException e) {}
In this case there is no leak, but added.
Line 514: _validateDigest(localDigest, new
String(remoteDigest.toByteArray(), "UTF-8").trim());
OK.
although the whole ***Stream are synchronous. So when the ssh library calls
close, the whole chain is closed.
Line 516: finally {
Outside class (application logic) should log.
This class should be pure infrastructure, I added debug though.
Line 517: t.interrupt();
But you don't know if it is finished or not. It may be in wait().
Line 537: ) throws Exception {
No need to repeat.
--
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: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Alon Bar-Lev <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Ofer Schreiber <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
