[libvirt] [PATCH] Log virRun command output, return with error

[Chris Lalancette]

Cole Robinson wrote:
> The attached patch adds some extra logging to the
> virRun command. The full argv is now logged with
> DEBUG, as well as the commands stdout and stderr
> (if anything is found).
> 
> Also, if the command returns an error, we raise
> the stderr content with the reported error msg.
> This will significantly help with debugging certain
> issues, particularly with the storage driver which
> makes heavy use of virRun.

I think the idea is great.  I'm a little worried about the implementation, though.

> --- a/src/util.c
> +++ b/src/util.c
> @@ -54,6 +54,9 @@
>  #include "memory.h"
>  #include "util-lib.c"
>  
> +#define DEBUG(fmt,...) VIR_DEBUG(__FILE__, fmt, __VA_ARGS__)
> +#define DEBUG0(msg) VIR_DEBUG(__FILE__, "%s", msg)

Leftover cruft, it looks like.  It's not used anywhere, and you can just use
DEBUG() for the same thing.

> +
>  #ifndef NSIG
>  # define NSIG 32
>  #endif
> @@ -404,10 +407,19 @@ int
>  virRun(virConnectPtr conn,
>         const char *const*argv,
>         int *status) {
> -    int childpid, exitstatus, ret;
> +    int childpid, exitstatus, ret, i;
> +    int errfd = -1, outfd = -1;
> +    char out[256] = "\0", err[256] = "\0", cmd[512] = "\0";;
> +
> +    /* Log argv */
> +    for (i = 0; argv[i] != NULL; ++i) {
> +        strncat(cmd, " ", sizeof(cmd) - strlen(cmd) - 1);
> +        strncat(cmd, argv[i], sizeof(cmd) - strlen(cmd) - 1);
> +    }

What happens if you have enough arguments that you overrun 512 bytes in the cmd
array?  I guess the sizeof(cmd) - strlen(cmd) - 1 will protect you from running
off the end of the buffer, but you'll truncate the output.  There are some
examples elsewhere in the code (src/qemu_conf.c, I think) that properly build up
the whole thing; it's probably worthwhile to look there.

> +    DEBUG("Running command '%s'", cmd);
>  
>      if ((ret = virExec(conn, argv, NULL, NULL,
> -                       &childpid, -1, NULL, NULL, VIR_EXEC_NONE)) < 0)
> +                       &childpid, -1, &outfd, &errfd, VIR_EXEC_NONE)) < 0)
>          return ret;
>  
>      while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR);
> @@ -418,16 +430,31 @@ virRun(virConnectPtr conn,
>          return -1;
>      }
>  
> +    /* Log command output */
> +    if (outfd) {
> +        ret = saferead(outfd, out, sizeof(out)-1);
> +        err[ret < 0 ? 0 : ret] = '\0';
> +        if (*out)
> +            DEBUG("Command stdout: %s", out);

Don't you also want to close(outfd) here?

> +    }
> +    if (errfd) {
> +        ret = saferead(errfd, err, sizeof(err)-1);
> +        err[ret < 0 ? 0 : ret] = '\0';
> +        if (*err)
> +            DEBUG("Command stderr: %s", err);

Ditto for errfd.

Chris Lalancette