Juan Hernandez has posted comments on this change.
Change subject: tools: engine-config bug fixes
......................................................................
Patch Set 4: (5 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/ConfigurationProvider.java
Line 65: out.close();
Please move the write down, so that it is performed only if the file has been
created and the permissions changed.
Line 66: // just in case, check if chmod exists...
Good idea. If it doesn't I think we should throw an exception:
File chmodFile = new File("/usr/bin/chmod");
if (!file.exists(chmodFile)) {
throw new IOException("Can't find the chmod command in order to change the
permissions of file \"" + filePath + "\".");
}
Line 68: Runtime.getRuntime().exec("/usr/bin/chmod " + filePath);
The code is not waiting for chmod to finish, so this may introduce a race
condition, not that important in this particular case, but it is better to take
care of it:
Process chmodProcess = Runtime.getRuntime().exec(chmodFile.getAbsoultePath()
+ " 600 " + filePath);
int chmodExitCode = chmodProcess.waitFor();
if (chmodExitCode != 0) {
throw new IOException("Failed to change permissions for file \"" +
filePath + "\", the chmod command returns exit code " + chmodExitCode + "\".);
}
Line 71: return filePath;
I still think that it is better to return the java.io.File object, instead of
the string.
Line 106: File file = new File(filePath);
If you use the java.io.File object, instead of String, you save the creation of
another File instance here.
--
To view, visit http://gerrit.ovirt.org/6096
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3003f20a51bb5a62387593d914894d62b012c70b
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Shahar Havivi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
