Juan Hernandez has posted comments on this change.
Change subject: tools: engine-config bug fixes
......................................................................
Patch Set 10: (3 inline comments)
See the comments in line.
In general it is advisable to avoid running external programs from Java
programs, as it is very expensive operation: it means a fork, and then the
kernel has to duplicate the page tables of the usually huge Java program, then
discard most of them to exec a usually tiny native program.
Doing it once for "chmod" is unavoidable (unless you use Java 7 NIO support, or
native code, or JNA) but anything else should be avoided.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/ConfigurationProvider.java
Line 63: Process proc = Runtime.getRuntime().exec("which chmod");
This means that you trust the "which" command is in the path. In that case you
can also assume that "chmod" is the path, in which case you can avoid this
method.
If you really, really want to check that the chmod file exists in the path then
it is better to implement it in Java, something like this:
String command = "chmod";
String path = System.getenv("PATH");
for (String directory : path.split(":")) {
File file = new File(directory, command);
if (file.exists()) {
return file;
}
}
But the "exec" method is already doing this. I won't do it.
Line 69: return new File(chmodPath);
This code doesn't wait for the "which" process to finish. If you finally decide
to keep this method it should wait and check the exit code.
Line 80: File chmodFile = getChmodFile();
Lets keep this simple, put here "/bin/chmod" or just "chmod" and don't try to
check if the file exists. If it doesn't the execution will fail and you will
get an error anyway, something like this:
java.io.IOException: Cannot run program "chmod": error=2, No such file or
directory
at java.lang.ProcessBuilder.start(ProcessBuilder.java:1029)
at java.lang.Runtime.exec(Runtime.java:615)
at java.lang.Runtime.exec(Runtime.java:448)
at java.lang.Runtime.exec(Runtime.java:345)
--
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: 10
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]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
