Juan Hernandez has posted comments on this change.

Change subject: tools: engine-config bug fixes
......................................................................


Patch Set 3: (3 inline comments)

....................................................
File 
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/kerberos/ConfigurationProvider.java
Line 67:         return fileName;
I think it is better if you return the File object here instead of the String, 
that way the caller can use "getAbsolutePath()" to avoid problems if the path 
is not absolute.

Also, will this create a world readable file? It does by default. In java 6 you 
will be forced to use Runtime.getRuntime().exec("/usr/bin/chmod", ...) to do 
it. In Java 7 you can use java.nio.file.Files.createTempFile(...).

Line 81:                                     " --admin-pass-file " + fileName)
If the file name is not absolute this will work/fail depending on what is the 
current working directory of the tool. Should be use explicitly an absolute 
path.

Line 82:                             + " -p " + engineConfigProperties);
Same for the config, although I think it is an absolute path already.

--
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: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi <shav...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to