Juan Hernandez has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 1: (48 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/hostinstall/MinaInstallWrapper.java
Line 26: static Log _log = LogFactory.getLog(MinaInstallWrapper.class);
Try to be explicit with the access modifiers. In this case "private" makes a
lot of sense.
Try to avoid the "_" to prefix field names, it is not a common practice in
Java, and makes the code harder to read.
Line 31: int _port = 22;
Make these private and remove the "_" prefix.
Line 35: _port = port;
If you are removing the "_" you can avoid ambiguity here with:
this.port = port;
Or even better (in my opinion):
public void setPort(int newPort) {
port = newPort;
}
Line 38: Credentials _prepareCredentials(String rootPassword) {
In general, make the methods private unless they need to have any other access
level.
Line 89: "_doConnect enter (%1$s, { %2$s, %3$s }, %4$d)",
This "_doConnect" information in the log message is redundant. The logging
mechanism can already give you the name of the method if configured propertly.
In addition when doing expensive operations to build messages (concatenating
strings, for example) is better to avoid them completely if the log level is
disabled:
if (log.isDebugEnabled()) {
log.debug(...);
}
Otherwise the computation of the message is performed regardless of what level
is activated, and this hurts performance.
This is probably not that important in this method, as it is not called that
often.
Line 118: ks.load(new FileInputStream(creds.getCertPath()),
pass);
This leaks one file descriptor. Try to do like this:
InputStream keystoreIn = null;
try {
keystoreIn = new FileInputStream(...);
ks.load(keystoreIn, pass);
}
finally {
try {
keystoreIn.close();
}
catch (IOException exception) {
log.error("Can't close keystore file.", exception);
}
}
Line 143: }
You are using ConfigValues.CertAlias several times in the previous code. You
could create temp variable to simplify things:
String alias = Config.<String>GetValue(ConfigValues.CertAlias);
Line 163: fingerprint = _fingerprint;
This is really hard to read, the names don't help.
Line 178: catch (Throwable t) {
Catching throwable is not a good a very good practice, it might be better to
catch Exception here.
Line 185: _log.debug(m, t);
I think this should be "_log.error(m, t)". In general the stack traces contain
very valuable information, and they should be logged. Using DEBUG level will
hide them most of the time.
The names "t" and "m" are not very self explanatory, better "exception" and
"message".
Line 199: }
Java doesn't have destructors, and finalizers might not be called, you should
not depend on them. If you really need to do cleanup before exit then it is
better to use a shutdown hook:
Runtime.getRuntime().addShutdownHook(new Thread() {
public void run() {
wrapperShutdown();
}
});
But in this case I think that who starts the install process should be
responsible from shutting it dow, probably in a "try ... finally" block.
Line 207: _log.debug("wrapperShutdown enter");
No need for method names in log messages.
Line 219: _log.debug("InitCallback enter");
No need for method names in log messages.
Line 265: class MessageOutputStream extends OutputStream {
Declaring non-anonymous classes inside methods is not common at all. Consider
moving the declaration outside of the method. Also I think this class is long
enough to have it in different file.
Line 274: for (int i=0;i<bbuffer.position();i++) {
Use spaces to make this easier to read:
for (int i = 0; i < buffer.position(); i++)
Line 282: String m;
What does "m" stand for?
Line 287: m = e.getMessage();
Don't swallow the exception stack trace, report it in the log.
Line 308: public void finallize() {
Please don't use finalize (in this case it is misspelled).
Line 318: catch (UnsupportedEncodingException e) {
Don't swallow the exception stack trace, report it in the log.
Line 400: _log.debug(m, e);
Don't hide the stack trace in the DEBUG level.
Line 412: _log.debug(m, t);
Don't hide the stack trace in the DEBUG level.
Line 441: _log.debug(m, e);
Don't hide the stack trace in the DEBUG level.
Line 444: catch (Throwable t) {
Consider catching Exception instead of Throwable.
Line 453: _log.debug(m, t);
Don't hide the stack trace in the DEBUG level.
Line 457: _log.debug("UploadFile leave " + ret);
No need for method names in log messages.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ssh/OvirtSSH.java
Line 32: public class OvirtSSH {
The name of the project is not usually added to class names, we already have it
in the package. I would suggest to use a different name here, something that
conveys the usage of this class. SecureShellClient for example.
Line 34: static final String COMMAND_FILE_SEND = "%1$s > '%2$s' && md5sum
-b '%2$s' | cut -d ' ' -f 1 >&2";
Make these private.
Line 45: PublicKey _serverKey;
Make these private and remove the "_".
Line 50: public static class ConstraintByteArrayOutputStream extends
ByteArrayOutputStream {
Move this class to its own file.
Line 100: OvirtSSH _client;
As this is a non static inner class this is not needed, you can use
"OvirtSSH.this".
Line 140: false
Quite an uncommon syntax trick in the Java world.
Line 149: public void finalize() {
Don't use finalize, it is unreliable, will be called when the garbage collector
decides, if at all. In fact most times it is not called at all.
Line 292: * The underlined ssh library does not provide
s/underlined/underlying/
Line 295: class IndexOutputStream extends FilterOutputStream {
These two classes might be useful outside this context. Move them to their own
files (that also helps making this clase easier to understand). The names could
be something like "ProgressInputStream" and "ProgressOutputStream", to reflect
better what they do.
Line 455: catch (IllegalArgumentException e) {
Don't swallow the exception stack trace, send it to the log or add it to the
new exception.
Line 476: ) throws Exception {
This "throw everything" usually means that you are not taking care of the
exceptions inside the method. I would suggest to throw here IOException, and
then take care of the other exceptions within the code, at least logging them.
Line 483: final MessageDigest localDigest =
MessageDigest.getInstance("MD5");
The message digest is not thread safe and you are using it from the new thread
and from the current thread. How do you make sure it will work correctly? I
would suggest to create a new message digest inside the runnable. You can add a
method to that runnable to return the resulting byte[].
Line 485: Thread t = new Thread(
Try to find a better name for the "t" variable.
It is also good if you use the Thread constructor that takes the runnable and a
thread name, that way it the threads blocks or accumulate is it easier to
figure out what is happening. In this case as you have the names of the files
it would be nice to include that in the name:
Runnable compressTask = new Runnable() {
...
}
Thread compressThread = new Thread(compressRunnabke, "Compress ...");
I also wonder what is the benefit of compressing as we are in a LAN.
Line 489: GZIPOutputStream zout = new
GZIPOutputStream(pout);
Consider using something like this:
OutputStream out = new DigestOutputStream(new GZIPOutputStream(...), digest);
Line 490: byte [] b = new byte[1024];
The typical way to declare arrays in java is "byte[]", no spaces beween the
type name and the square brackets.
When reading/writing from/to files (specially large files) 1024 bytes is quite
small, 4096 (a page) is probably better, maybe even larger.
Line 498: catch (IOException e) {}
Don't swallow exceptions, at least report them in the log.
If something fails inside the "try" you will be leaking the output stream, and
probably a file descriptor. Move "zout.close()" to a "finally" block.
Line 514: _validateDigest(localDigest, new
String(remoteDigest.toByteArray(), "UTF-8").trim());
When you get here the thread that compresses may be still running and using the
localDigest, you shouldn't use it here unless you are completely sure that the
compress thread won't do it simultaneously. I would suggest to wait for the
thread (probably with a timeout) before validating the digest:
t.join(timeout);
Then, if it doesn't finish you may interrupt it here.
Line 516: finally {
I miss "catch" here. At least to catch the exceptions and send an explanatory
message to the log.
Line 517: t.interrupt();
No need to interrupt the thread if it already finished.
Line 537: ) throws Exception {
Same as before: does this mean that you are not taking care of the exceptions
within the method?
Line 544: final MessageDigest localDigest =
MessageDigest.getInstance("MD5");
Same as before, be careful with concurrent access to this object.
Line 549: GZIPInputStream zin = new
GZIPInputStream(pin2);
Consider using DigestInputStream.
Line 574: _validateDigest(localDigest, new
String(remoteDigest.toByteArray(), "UTF-8").trim());
Wait for the thread to finish before using localDigest.
--
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
