On Fri, Mar 26, 2010 at 10:53:01AM -0600, Eric Blake wrote:
On 03/26/2010 09:43 AM, Daniel Veillard wrote:
+ if (stat(path, &sb) < 0) { + ret = 0; + VIR_DEBUG("No hook script %s", path); + } else { + if (access(path, X_OK) != 0) {
Should we also check for !S_ISDIR(&sb.st_mode), so that we explicitly reject directories here, rather than failing later when trying to execute them? Or go one step further and require regular files, with the stricter check for S_ISREG(&sb.st_mode)? (Note: symlinks to regular files would still be okay, given that you used stat().)
Right, I'm adding this
+ * virHookInitialize: + * + * Initialize syncronous hooks support.
s/syncronous/synchronous/
+/** + * virHookPresent: + * @driver: the driver number (from virHookDriver enum) + * + * Check if a hook exists for the given driver, this is needed + * to avoid unecessary work if the hook is not present
s/unecessary/unnecessary/
+/* + * virHookCall: + * @driver: the driver number (from virHookDriver enum) + * @id: an id for the object '-' if non available for example on daemon hooks + * @op: the operation on the id e.g. VIR_HOOK_QEMU_OP_START + * @sub_op: a sub_operation, currently unused + * @extra: optional string informations
s/informations/information/ (multiple times)
+ * @input: extra input given to the script on stdin + * + * Implement an Hook call, where the external script for the driver is
s/an Hook/a Hook/ English is funny on 'a' vs. 'an' before a word starting in 'h'; sometimes you just have to use a native speaker to get it right ;)
fixed the typos, thanks for raising them, btw I would normally spot 'an hook' as a problem, but I guess I didn't reread thoise comments, thanks for spotting those !
+ /* + * Convenience macros borrowed from qemudBuildCommandLine() + */
Duplicating the definition of all these helpers is a bit of a long distraction in the middle of this function; is it worth a helper file, where we could #include "command_line_builder.h" to get all these helpers defined, and to reduce the duplication from qemudBuildCommandLine by having the macros in one place?
We can clean this up as Dan suggested, but that should be done separately, there is a few place in the code where we do this
+ ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, + VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); + close(pipefd[1]);
Should we be checking for close() failures, and reporting a system error?
yes, but not changing behaviour, I fixed those allowed me to spot that if (pipefd[0] > 0) test on function exit should really be >= as 0 is a valid fd, so fixed those too
I only saw minor problems as pointed out above; the overall patch looks technically sound.
thanks, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/