Re: [libvirt] [PATCH] [3/6] Add hook utilities

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().)

> + * 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 ;)

> + /* > + * 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?

> + 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?

I only saw minor problems as pointed out above; the overall patch looks technically sound.