[libvirt] [PATCH] Improve error reporting in virsh

# HG changeset patch # User john.levon@sun.com # Date 1233859425 28800 # Node ID c594c1c54b88947082a01b6aaa81ad7756bbd951 # Parent a23da778e0ddf954d8bf0a185b508fc236feb4be Improve error reporting in virsh Rather than verbosely printing every error, save the last error and report that only if the entire command fails. Signed-off-by: John Levon <john.levon@sun.com> diff --git a/src/virsh.c b/src/virsh.c --- a/src/virsh.c +++ b/src/virsh.c @@ -93,22 +93,6 @@ typedef enum { } vshErrorLevel; /* - * The error handler for virsh - */ -static void -virshErrorHandler(void *unused, virErrorPtr error) -{ - if ((unused != NULL) || (error == NULL)) - return; - - /* Suppress the VIR_ERR_NO_XEN error which fails as non-root */ - if ((error->code == VIR_ERR_NO_XEN) || (error->code == VIR_ERR_OK)) - return; - - virDefaultErrorFunc(error); -} - -/* * virsh command line grammar: * * command_line = <command>\n | <command>; <command>; ... @@ -319,6 +303,45 @@ static int namesorter(const void *a, con const char **sb = (const char**)b; return strcasecmp(*sa, *sb); +} + +static virErrorPtr last_error; + +/* + * Quieten libvirt until we're done with the command. + */ +static void +virshErrorHandler(void *unused, virErrorPtr error) +{ + last_error = virSaveLastError(); + if (getenv("VIRSH_DEBUG") != NULL) + virDefaultErrorFunc(error); +} + +/* + * Report an error when a command finishes. This is better than before + * (when correct operation would report errors), but it has some + * problems: we lose the smarter formatting of virDefaultErrorFunc(), + * and it can become harder to debug problems, if errors get reported + * twice during one command. This case shouldn't really happen anyway, + * and it's IMHO a bug that libvirt does that sometimes. + */ +static void +virshReportError(vshControl *ctl) +{ + if (last_error == NULL) + return; + + if (last_error->code == VIR_ERR_OK) { + vshError(ctl, FALSE, "%s", _("unknown error")); + goto out; + } + + vshError(ctl, FALSE, "%s", last_error->message); + +out: + free(last_error); + last_error = NULL; } @@ -6102,6 +6125,9 @@ vshCommandRun(vshControl *ctl, const vsh if (ctl->timing) GETTIMEOFDAY(&after); + + if (ret == FALSE) + virshReportError(ctl); if (STREQ(cmd->def->name, "quit")) /* hack ... */ return ret;

On Thu, Feb 05, 2009 at 11:28:48AM -0800, john.levon@sun.com wrote:
Improve error reporting in virsh
Rather than verbosely printing every error, save the last error and report that only if the entire command fails.
This version fixes up the memory leak issue Dan spotted. It's dependent upon the virSaveLastError() patch (and yes, I'll fix up that changelog :) regards, john

On Thu, Feb 05, 2009 at 11:28:48AM -0800, john.levon@sun.com wrote:
# HG changeset patch # User john.levon@sun.com # Date 1233859425 28800 # Node ID c594c1c54b88947082a01b6aaa81ad7756bbd951 # Parent a23da778e0ddf954d8bf0a185b508fc236feb4be Improve error reporting in virsh
Rather than verbosely printing every error, save the last error and report that only if the entire command fails.
+/* + * Report an error when a command finishes. This is better than before + * (when correct operation would report errors), but it has some + * problems: we lose the smarter formatting of virDefaultErrorFunc(), + * and it can become harder to debug problems, if errors get reported + * twice during one command. This case shouldn't really happen anyway, + * and it's IMHO a bug that libvirt does that sometimes. + */ +static void +virshReportError(vshControl *ctl) +{ + if (last_error == NULL) + return; + + if (last_error->code == VIR_ERR_OK) { + vshError(ctl, FALSE, "%s", _("unknown error")); + goto out; + } + + vshError(ctl, FALSE, "%s", last_error->message); + +out: + free(last_error); + last_error = NULL; }
This actually still leaks the char * strings associated with the error - we probably need to now add a virErrorFree() function to go along with the new virSaveError function. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Thu, Feb 05, 2009 at 08:20:28PM +0000, Daniel P. Berrange wrote:
This actually still leaks the char * strings associated with the error - we probably need to now add a virErrorFree() function to go along with the new virSaveError function.
Yep, working on two new patches. Sorry about that. regards john
participants (4)
-
Daniel P. Berrange
-
John Levon
-
John Levon
-
john.levon@sun.com