Re: [libvirt] [PATCH 1/3] Introduce new APIs for spawning processes

On 05/25/2010 07:24 AM, Daniel P. Berrange wrote: > This introduces a new set of APIs in src/util/command.h > to use for invoking commands. This is intended to replace > all current usage of virRun and virExec variants, with a > more flexible and less error prone API. > --- > src/Makefile.am | 1 + > src/util/command.c | 782 ++++++++++++++++++++++++++++++++++++++++++ > src/util/command.h | 197 +++++++++++ > tests/Makefile.am | 14 +- > tests/commanddata/test1.log | 12 + > tests/commanddata/test10.log | 14 + > tests/commanddata/test11.log | 12 + > tests/commanddata/test12.log | 12 + > tests/commanddata/test13.log | 12 + > tests/commanddata/test2.log | 14 + > tests/commanddata/test3.log | 12 + > tests/commanddata/test4.log | 10 + > tests/commanddata/test5.log | 6 + > tests/commanddata/test6.log | 11 + > tests/commanddata/test7.log | 7 + > tests/commanddata/test8.log | 14 + > tests/commanddata/test9.log | 14 + > tests/commandhelper.c | 112 ++++++ > tests/commandtest.c | 494 ++++++++++++++++++++++++++ Nice to see as much test additions as the code itself. > + > +struct _virCommand { > + int has_error; s/int/bool/? > + > + int nargs; s/int/size_t/ > + char **args; > + > + int nenv; s/int/size_t/ > + char **env; > + > + fd_set preserve; This implicitly limits us to FD_SETSIZE fd's (aka 32 on some platforms); is that acceptable, or should we be using a better structure?

> + int outfd; > + int errfd; > + int *outfdptr; > + int *errfdptr; > + > + virExecHook hook; > + void *opaque; > + > + pid_t pid; > + char *pidfile; Do we want to allow the parent process to control which directory the child will start in (defaulting to the parent's cwd)?

> +/* > + * Re-allow a specific capability > + */ > +void virCommandAllowCap(virCommandPtr cmd, > + int capability ATTRIBUTE_UNUSED) > +{ > + if (!cmd || cmd->has_error) > + return; > + > + /* XXX ? */ > +} Do we have a use case for this yet, or should we just #if 0 this section until there is a need and a reasonable implementation?

> +void virCommandAddEnvPair(virCommandPtr cmd, > + const char *name, > + const char *value) > +{ > + char *env; > + > + if (!cmd || cmd->has_error) > + return; > + > + if (virAsprintf(&env, "%s=%s", name, value ? value : "") < 0) { > + cmd->has_error = 1; > + return; > + } > + > + if (VIR_REALLOC_N(cmd->env, cmd->nenv + 2) < 0) { This scales quadratically, and can lead to noticeable performance degredation if there are lots of calls to virCommandAddEnvPair. Better would be to amortize the allocation costs by tracking the allocation size of cmd->env (in addition to cmd->nenv), and geometrically enlarging it as-needed (by a factor of 1.5x or 2x), rather than a reallocation on every addition.

> + > +/* > + * Add a NULL terminated list of args > + */ > +void virCommandAddArgSet(virCommandPtr cmd, > + const char *const*vals) Is it worth a varargs variant of this: virCommandAddArgList(cmd, "a", "b", NULL)?

> +/* > + * Feed the child's stdin from a string buffer > + */ > +void virCommandSetInputBuffer(virCommandPtr cmd, > + const char *inbuf) > +{ > + if (!cmd || cmd->has_error) > + return; > + > + cmd->inbuf = inbuf; > + cmd->infd = -1; > + cmd->inpipe = -1; Should we set has_error if the user calls both virCommandSetInputBuffer and virCommandSetInputFD? (Detectable if infd or inpipe are not -1 here; likewise detectable in virCommandSetInputFD if input is not NULL).

> +static int > +virCommandProcessIO(virCommandPtr cmd) Documentation? > +{ > + int infd = -1, outfd = -1, errfd = -1; > + size_t inlen = 0, outlen = 0, errlen = 0; > + size_t inoff = 0; > + > + /* With an input buffer, feed data to child > + * via pipe */ > + if (cmd->inbuf) { > + inlen = strlen(cmd->inbuf); > + infd = cmd->inpipe; > + } > + > + /* With out/err buffer, we're the outfd/errfd s/we're // > + * have been filled with an FD for us */ > + if (cmd->outbuf) > + outfd = cmd->outfd; > + if (cmd->errbuf) > + errfd = cmd->errfd; > + > + for (;;) { > + int i; > + struct pollfd fds[3]; > + int nfds = 0; > + > + if (infd != -1) { > + fds[nfds].fd = infd; > + fds[nfds].events = POLLOUT; > + nfds++; > + } > + if (outfd != -1) { > + fds[nfds].fd = outfd; > + fds[nfds].events = POLLIN; > + nfds++; > + } > + if (errfd != -1) { > + fds[nfds].fd = errfd; > + fds[nfds].events = POLLIN; > + nfds++; > + } Should this initialization be floated outside of the for(;;) loop?

> + > + if (nfds == 0) > + break; > + > + if (poll(fds,nfds, -1) < 0) { > + if ((errno == EAGAIN) || (errno == EINTR)) > + continue; > + virReportSystemError(errno, "%s", > + _("unable to poll on child")); > + return -1; > + } > + > + for (i = 0; i < nfds ; i++) { > + if (fds[i].fd == errfd || > + fds[i].fd == outfd) { > + char data[1024]; > + char **buf; > + size_t *len; > + int done; > + if (fds[i].fd == outfd) { > + buf = cmd->outbuf; > + len = &outlen; > + } else { > + buf = cmd->errbuf; > + len = &errlen; > + } > + > + done = read(fds[i].fd, data, sizeof(data)); > + if (done < 0) { > + if (errno != EINTR && > + errno != EAGAIN) { > + virReportSystemError(errno, "%s", > + _("unable to write to child input")); > + return -1; > + } > + } else if (done == 0) { > + if (fds[i].fd == outfd) > + outfd = -1; > + else > + errfd = -1; > + } else { > + if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) { > + virReportOOMError(); > + return -1; > + } > + memmove(*buf + *len, data, done); s/memmove/memcpy/ - there's no overlap between data and buf > + *len += done; > + (*buf)[*len] = '\0'; > + } > + } else { > + int done; > + > + done = write(infd, cmd->inbuf + inoff, > + inlen - inoff); > + if (done < 0) { > + if (errno != EINTR && > + errno != EAGAIN) { > + virReportSystemError(errno, "%s", > + _("unable to write to child input")); > + return -1; > + } > + } else { > + inoff += done; > + if (inoff == inlen) { > + close(infd); > + infd = -1; > + } > + } > + } > + > + } > + } > + > + return 0; > + > +}

> +/* > + * Run the command asynchronously > + * Returns -1 on any error executing the > + * command. Returns 0 if the command executed. > + */ > +int virCommandRunAsync(virCommandPtr cmd, > + pid_t *pid) > +{ > + int ret; > + char *str; > + > + if (!cmd || cmd->has_error) { > + virReportOOMError(); > + return -1; > + } > + > + if (cmd->pid != -1) { > + virCommandError(VIR_ERR_INTERNAL_ERROR, > + _("command is already running as pid %d"), > + cmd->pid); > + return -1; > + } > + > + str = virArgvToString((const char *const *)cmd->args); > + VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); > + VIR_FREE(str); > + > + ret = virExecWithHook((const char *const *)cmd->args, > + (const char *const *)cmd->env, > + &cmd->preserve, > + &cmd->pid, > + cmd->infd, > + cmd->outfdptr, > + cmd->errfdptr, > + cmd->flags, > + cmd->hook, > + cmd->opaque, > + cmd->pidfile); Should virExecWithHook be moved into this new file?

> +int main(int argc, char **argv) { > + int i, n; > + char **origenv; > + char **newenv; > + FILE *log = fopen(abs_builddir "/commandhelper.log", "w"); > + > + if (!log) > + goto error; > + > + for (i = 1 ; i < argc ; i++) { > + fprintf(log, "ARG:%s\n", argv[i]); > + } > + > + origenv = environ; > + n = 0; > + while (*origenv != NULL) { > + n++; > + origenv++; > + } > + > + if (VIR_ALLOC_N(newenv, n) < 0) { > + exit(EXIT_FAILURE); > + } > + > + origenv = environ; > + n = i = 0; > + while (*origenv != NULL) { > + newenv[i++] = *origenv; > + n++; Why set i = 0, and increment both i and n, when you could have just done newenv[n++] = *origenv > + origenv++; > + } > + qsort(newenv, n, sizeof(newenv[0]), envsort); > + > + for (i = 0 ; i < n ; i++) { > + fprintf(log, "ENV:%s\n", newenv[i]); > + } > + > + for (i = 0 ; i < sysconf(_SC_OPEN_MAX) ; i++) { Will this even compile on mingw? Gnulib provides getdtablesize() (oops, it's still LGPLv3) which portably lets you determine a nice maximum bound on the number of open files, even when sysconf() is not available.

> diff --git a/tests/commandtest.c b/tests/commandtest.c > new file mode 100644 > index 0000000..c66d345 > --- /dev/null > +++ b/tests/commandtest.c > @@ -0,0 +1,494 @@ > +#include <config.h> > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > +#include <signal.h> > + > +#include "testutils.h" > +#include "internal.h" > +#include "nodeinfo.h" > +#include "util.h" > +#include "memory.h" > +#include "command.h" > + > +#ifndef __linux__ Is this test really linux-specific? It seems like other capable platforms, like cygwin, might be able to run it just fine.

> +static int > +mymain(int argc, char **argv) > +{ > + int ret = 0; > + char cwd[PATH_MAX]; > + > + abs_srcdir = getenv("abs_srcdir"); > + if (!abs_srcdir) > + abs_srcdir = getcwd(cwd, sizeof(cwd)); > + > + progname = argv[0]; > + > + if (argc > 1) { > + fprintf(stderr, "Usage: %s\n", progname); > + return(EXIT_FAILURE); > + } > + > + if (chdir("/tmp") < 0) > + return(EXIT_FAILURE); Do we really want to risk spurious test failures by running directly in /tmp, or should we be creating a secure subdirectory?