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 ;)
> + * called with the given informations. This is a synchronous call, we wait for
> + * execution completion
> + *
> + * Returns: 0 if the execution suceeded, 1 if the script was not found or
s/suceeded/succeeded/
> + * invalid parameters, and -1 if script returned an error
> + */
> +int
> +virHookCall(int driver, const char *id, int op, int sub_op, const char *extra,
> + const char *input) {
> + int ret, waitret, exitstatus, i;
> + char *path;
> + int argc = 0, arga = 0;
> + const char **argv = NULL;
> + int envc = 0, enva = 0;
> + const char **env = NULL;
> + const char *drvstr;
> + const char *opstr;
> + const char *subopstr;
> + pid_t pid;
> + int outfd = -1, errfd = -1;
> + int pipefd[2] = { -1, -1};
> + char *outbuf = NULL;
> + char *errbuf = NULL;
> +
> + if ((driver < VIR_HOOK_DRIVER_DAEMON) ||
> + (driver >= VIR_HOOK_DRIVER_LAST))
> + return(1);
> +
> + /*
> + * We cache the availability of the script to minimize impact at
> + * runtime if no script is defined, this is being reset on SIGUP
s/SIGUP/SIGHUP/
> + */
> + if ((virHooksFound == -1) ||
> + ((driver == VIR_HOOK_DRIVER_DAEMON) &&
> + (op == VIR_HOOK_DAEMON_OP_RELOAD)))
> + virHookInitialize();
> +
...
> + /*
> + * 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?
> + ADD_ENV_LIT("LC_ALL=C");
> +
> + ADD_ENV_COPY("LD_PRELOAD");
> + ADD_ENV_COPY("LD_LIBRARY_PATH");
> + ADD_ENV_COPY("PATH");
> + ADD_ENV_COPY("HOME");
> + ADD_ENV_COPY("USER");
> + ADD_ENV_COPY("LOGNAME");
> + ADD_ENV_COPY("TMPDIR");
> + ADD_ENV(NULL);
Not new to your patch, but POSIX 2008 recommends that we should really
be checking confstr(_CS_V7_ENV), when that exists, for any other
environment variables that we might also need to copy to keep the child
process in an environment equally as conforming as the parent (for
example, whether POSIXLY_CORRECT needs to be propagated). Probably not
worth worrying about, though, unless someone identifies an actual bug.
> +
> + ADD_ARG_LIT(path);
> + ADD_ARG_LIT(id);
> + ADD_ARG_LIT(opstr);
> + ADD_ARG_LIT(subopstr);
> +
> + ADD_ARG_LIT(extra);
> + ADD_ARG(NULL);
> +
> + /* pass any optional input on the script stdin */
> + if (input != NULL) {
> + if (pipe(pipefd) < -1) {
> + virReportSystemError(errno, "%s",
> + _("unable to create pipe for hook input"));
> + ret = 1;
> + goto cleanup;
> + }
> + if (safewrite(pipefd[1], input, strlen(input)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("unable to write to pipe for hook input"));
> + ret = 1;
> + goto cleanup;
> + }
> + 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.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list