Juan Hernandez has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 1: (14 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)",
I understand the feeling :-) . Try to be positive, you will learn to love it
most of the time, and hate it from time to time as well.
Line 207: _log.debug("wrapperShutdown enter");
That is ok. For your information, there are better ways to do this, something
we may want to look at sometime in the future:
http://www.eclipse.org/aspectj/sample-code.html#tracing-traceJoinPoints
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/OvirtSSH.java
Line 32: public class OvirtSSH {
That is fine, thanks.
Line 45: PublicKey _serverKey;
Thanks.
Line 50: public static class ConstraintByteArrayOutputStream extends
ByteArrayOutputStream {
Thanks, it makes this file shorter. Two short files are easier to understand
than one large one, in my opinion.
Line 140: false
Yes, agree.
Line 149: public void finalize() {
So do we have another place where the "disconnect" method is being called
explicitly? If that is the case then it is ok to leave it here.
Just to make the problems with finalize clearer I suggest you try the following
code:
public class Test {
public Test() {
System.out.println("init");
}
public void finalize() {
System.out.println("finalize");
}
public static void main(String[] args) {
new Test();
}
}
What will be the output?
Line 295: class IndexOutputStream extends FilterOutputStream {
Thanks, it makes this file shorter.
Line 476: ) throws Exception {
Fair enough.
Line 485: Thread t = new Thread(
Ok, understood.
Line 490: byte [] b = new byte[1024];
Need to increase the size used by the pipe streams as well:
new PipedInputStream(8192);
Line 498: catch (IOException e) {}
There is a leak if "fin.read(...)", "localDigest.update(...)" or
"zout.write(...)" fail, for whatever the reason. A typical reason is someone
interrupting the thread with the "Thread.interrupt()" method.
Line 514: _validateDigest(localDigest, new
String(remoteDigest.toByteArray(), "UTF-8").trim());
I meant something like this, as waiting for a thread to complete its task is
always tricky:
try {
t.join(timeout);
if (t.getState() == Thread.State.TERMINATED) {
// This is the only place where you can tell for sure that the thread
// finished, but you need to be careful to consume the results, as
// threads need sychronization in order to have a consistent view
// of memory.
}
else {
log.error("Sender thread didn't finish after the timeout.");
}
}
catch (InterruptedException exception) {
log.error("Interrupted while waiting for sender thread to finish.",
exception);
}
If you want to avoid this tricky business you can try to use one of the
executors provided by the java.util.concurrent package:
// Find a service, probably reusing it:
ExecutorService service = Executors.new...;
// Create the task to be executed that sends the file and calculates
// the digest:
Callable<byte[]> sendFileTask = new Callable<byte[]>() {
public byte[] call() throws Exception {
// Send the file, compute the digest and return the result:
byte[] digestBytes = ...;
return digestBytes;
}
};
// Submit the task for future execution:
Future<byte[]> futureResults = service.submit(sendFileTask);
// Do other things, like invoking the SSH client to execute the command:
...
// Wait for the send file task to complete, with a timeout:
try {
byte[] digestBytes = futureResults.get(1000, TimeUnit.SECONDS);
// Here you know that the task finished and can just consume the
// results, no special synchronization needed.
}
catch (...) {
}
Regarding the "close" of the stream take into account that the
PipedInputStream<->PipedOutputStream that you are using insulates both sides of
the chain: closing one side will not close the other. So even if the SSH
library closes its side you are still responsible for closing the other side.
If you use the "try ... finally" idiom in the thread then there is no problem,
the stream will always be closed.
Line 517: t.interrupt();
Fair enough.
--
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 <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
