[libvirt] [PATCH] util: include stderr in log message when an external command fails

[Daniel P. Berrange]

On Mon, Aug 06, 2012 at 08:07:32PM -0400, Laine Stump wrote:
> This patch is in response to:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=818467
> 
> If a caller to virCommandRun doesn't ask for the exitstatus of the
> program it's running, the virCommand functions assume that they should
> log an error message and return failure if the exit code isn't
> 0. However, only the commandline and exit status are logged, while
> potentially useful information sent by the program to stderr is
> discarded.
> 
> Fortunately, virCommandRun is already checking if the caller had asked
> for stderr to be saved and, if not, sets things up to save it in
> *cmd->errbuf. This makes it fairly simple for virCommandWait to
> include *cmd->errbuf in the error log (there are still other callers
> that don't setup errbuf, and even virCommandRun won't set it up if the
> command is being daemonized, so we have to check that it's non-zero).
> ---
> 
> Note that the minor change to the first virReportError string was made
> because I noticed that virCommandTranslateStatus already puts the word
> "status" in its string. The new message is less awkward.
> 
>  src/util/command.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/command.c b/src/util/command.c
> index 334ca89..7755572 100644
> --- a/src/util/command.c
> +++ b/src/util/command.c
> @@ -2269,7 +2269,7 @@ virPidWait(pid_t pid, int *exitstatus)
>          if (status != 0) {
>              char *st = virCommandTranslateStatus(status);
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Child process (%lld) status unexpected: %s"),
> +                           _("Child process (%lld) unexpected %s"),
>                             (long long) pid, NULLSTR(st));
>              VIR_FREE(st);
>              return -1;
> @@ -2327,9 +2327,13 @@ virCommandWait(virCommandPtr cmd, int *exitstatus)
>          if (status) {
>              char *str = virCommandToString(cmd);
>              char *st = virCommandTranslateStatus(status);
> +            bool haveErrMsg = cmd->errbuf && *cmd->errbuf && (*cmd->errbuf)[0];
> +
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("Child process (%s) status unexpected: %s"),
> -                           str ? str : cmd->args[0], NULLSTR(st));
> +                           _("Child process (%s) unexpected %s%s%s"),
> +                           str ? str : cmd->args[0], NULLSTR(st),
> +                           haveErrMsg ? ": " : "",
> +                           haveErrMsg ? *cmd->errbuf : "");
>              VIR_FREE(str);
>              VIR_FREE(st);
>              return -1;

ACK


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|