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 :|