Alon Bar-Lev has posted comments on this change.
Change subject: bootstrap: new implementation for apache-sshd usage
......................................................................
Patch Set 1: (13 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);
Well, I am not native Java speaker... I will do whatever you want me to do in
these styles check.
BUT...
1. There should be a convention to distinguish between member variables and
local variables, so it is easier to understand what come from what. So I think
that it does help code readability, but I guess it is a matter of choice.
2. As far as I know Java states everything not "public" to be protected from
outside would, why lose inherit advantage and explicit mark as private?
Line 35: _port = port;
I disagree I think the _ convention is much more clear :)
But I don't mind...
Line 38: Credentials _prepareCredentials(String rootPassword) {
Why lose the flexibility?
Anyway, please notice that the naming of private members and private functions
are consistent... the '_' prefix on member functions also helps code
readability.
Line 118: ks.load(new FileInputStream(creds.getCertPath()),
pass);
OK.
Line 143: }
OK.
Line 163: fingerprint = _fingerprint;
OK.
Line 178: catch (Throwable t) {
Can you please explain? I never heard this one before....
Line 185: _log.debug(m, t);
This was the way before... (most of the times) I did not change this.
I don't like error messages for users to contain debug information, but I will
change this and remove the debug.
the "t" "m" is also a matter of taste, in 3 line code... well, I will change.
Line 207: _log.debug("wrapperShutdown enter");
Why not?
How do we trace enter/leave?
Line 265: class MessageOutputStream extends OutputStream {
Used as helper in single function, so it belongs to the function.
Here I disagree strongly.
Line 274: for (int i=0;i<bbuffer.position();i++) {
OK.
Although...
Line 287: m = e.getMessage();
This is intentional, as the uncodeing exception likely never happen, and in the
chance of it does happen I display the exception as message.
Line 308: public void finallize() {
OK, I will remove them all.
--
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
