[libvirt] [PATCH 08/10] Introduce new APIs for spawning processes

[Daniel P. Berrange]

On Wed, Nov 17, 2010 at 09:29:00PM -0700, Eric Blake wrote:
> From: Daniel P. Berrange <berrange at redhat.com>
> 
> 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/util/command.c: New file.
> * src/util/command.h: New header.
> * src/Makefile.am (UTIL_SOURCES): Build it.
> * src/libvirt_private.syms: Export symbols internally.
> * tests/commandtest.c: New test.
> * tests/Makefile.am (check_PROGRAMS): Run it.
> * tests/commandhelper.c: Auxiliary program.
> * tests/commanddata/test2.log - test15.log: New expected outputs.
> * cfg.mk (useless_free_options): Add virCommandFree.
> * po/POTFILES.in: New translation.
> * .x-sc_avoid_write: Add exemption.
> * tests/.gitignore: Ignore new built file.


> +struct _virCommand {
> +    int has_error; /* ENOMEM on allocation failure, -1 for anything else.  */
> +
> +    char **args;
> +    size_t nargs;
> +    size_t maxargs;
> +
> +    char **env;
> +    size_t nenv;
> +    size_t maxenv;
> +
> +    char *pwd;
> +
> +    /* XXX Use int[] if we ever need to support more than FD_SETSIZE fd's.  */
> +    fd_set preserve;
> +    unsigned int flags;
> +
> +    char *inbuf;
> +    char **outbuf;
> +    char **errbuf;
> +
> +    int infd;
> +    int inpipe;
> +    int outfd;
> +    int errfd;
> +    int *outfdptr;
> +    int *errfdptr;
> +
> +    virExecHook hook;
> +    void *opaque;
> +
> +    pid_t pid;
> +    char *pidfile;
> +};
> +/*
> + * Release all resources
> + */
> +void
> +virCommandFree(virCommandPtr cmd)
> +{
> +    int i;
> +    if (!cmd)
> +        return;
> +
> +    VIR_FORCE_CLOSE(cmd->outfd);
> +    VIR_FORCE_CLOSE(cmd->errfd);
> +
> +    for (i = 0 ; i < cmd->nargs ; i++)
> +        VIR_FREE(cmd->args[i]);
> +    VIR_FREE(cmd->args);
> +
> +    for (i = 0 ; i < cmd->nenv ; i++)
> +        VIR_FREE(cmd->env[i]);
> +    VIR_FREE(cmd->env);
> +
> +    VIR_FREE(cmd->pwd);
> +
> +    VIR_FREE(cmd->pidfile);
> +
> +    VIR_FREE(cmd);
> +}


My code forgot to ever close() the fds in cmd->preserve. We definitely
need todo it in virCommandFree(), but there's a small argument to say
we should also do it in virCommandRun/virCommandRunAsync so that if
the caller keeps the virCommandPtr alive for a long time, we don't
have the open FDs.

It would also be useful to have a generic API for logging info about
the command to an FD (to let us remove that logging code from UML
and QEMU & LXC drivers).

eg

+void virCommandWriteArgLog(virCommandPtr cmd, int logfd)
+{
+    int ioError = 0;
+    int i;
+
+    for (i = 0 ; i < cmd->nenv ; i++) {
+        if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0)
+            ioError = errno;
+        if (safewrite(logfd, " ", 1) < 0)
+            ioError  = errno;
+    }
+    for (i = 0 ; i < cmd->nargs ; i++) {
+        if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0)
+            ioError = errno;
+        if (safewrite(logfd, " ", 1) < 0)
+            ioError  = errno;
+    }
+
+    if (ioError) {
+        char ebuf[1024];
+        VIR_WARN("Unable to write command %s args to logfile: %s",
+                 cmd->args[0], virStrerror(ioError, ebuf, sizeof ebuf));
+    }
+}


Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|