[libvirt] [PATCH] Improve error reporting in virsh

[Daniel P. Berrange]

On Wed, Feb 04, 2009 at 03:56:26PM -0800, john.levon at sun.com wrote:
> # HG changeset patch
> # User john.levon at sun.com
> # Date 1233791330 28800
> # Node ID 6588879d8cc9a3af1147a5edd624aee5155493ae
> # Parent  da162569b5f7e132c4ccbfc56fc3670fb5ecee10
> Improve error reporting in virsh
> 
> Rather than verbosely printing every error, save the last error and
> report that only if the entire command fails. Additionally, avoid
> over-writing an existing error in various places in the library.
> 
> Signed-off-by: John Levon <john.levon at sun.com>


> diff --git a/src/domain_conf.c b/src/domain_conf.c
> --- a/src/domain_conf.c
> +++ b/src/domain_conf.c
> @@ -2432,8 +2432,7 @@ catchXMLError (void *ctx, const char *ms
>      if (ctxt) {
>          virConnectPtr conn = ctxt->_private;
>  
> -        if (conn &&
> -            conn->err.code == VIR_ERR_NONE &&
> +        if (virGetLastError() == NULL &&
>              ctxt->lastError.level == XML_ERR_FATAL &&
>              ctxt->lastError.message != NULL) {
>              virDomainReportError (conn, VIR_ERR_XML_DETAIL,
> @@ -2466,7 +2465,7 @@ virDomainDefPtr virDomainDefParseString(
>                            XML_PARSE_NOENT | XML_PARSE_NONET |
>                            XML_PARSE_NOWARNING);
>      if (!xml) {
> -        if (conn && conn->err.code == VIR_ERR_NONE)
> +        if (virGetLastError() == NULL)
>                virDomainReportError(conn, VIR_ERR_XML_ERROR,
>                                     "%s", _("failed to parse xml document"));
>          goto cleanup;
> @@ -2507,7 +2506,7 @@ virDomainDefPtr virDomainDefParseFile(vi
>                             XML_PARSE_NOENT | XML_PARSE_NONET |
>                             XML_PARSE_NOWARNING);
>      if (!xml) {
> -        if (conn && conn->err.code == VIR_ERR_NONE)
> +        if (virGetLastError() == NULL)
>                virDomainReportError(conn, VIR_ERR_XML_ERROR,
>                                     "%s", _("failed to parse xml document"));
>          goto cleanup;

ACK to those bits.

> @@ -319,6 +303,41 @@ 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 = virCloneError(error);
> +    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, TRUE, "%s", _("unknown error"));
> +        return;
> +    }
> +
> +    vshError(ctl, TRUE, "%s", last_error->message);
>  }


Since you only ever print out the 'last_error->message'
field here, I think its better to just do strdup() of
that field, instead of adding a new virCloneError API.
Also, it is neccessary to free the error message/object
after reporting it to avoid a memory leak.

> diff --git a/src/xend_internal.c b/src/xend_internal.c
> --- a/src/xend_internal.c
> +++ b/src/xend_internal.c
> @@ -105,12 +105,16 @@ virDomainDevID(virDomainPtr domain,
>                 int devid_len);
>  #endif
>  
> -#define virXendError(conn, code, fmt...)                                     \
> -        virReportErrorHelper(conn, VIR_FROM_XEND, code, __FILE__,          \
> -                               __FUNCTION__, __LINE__, fmt)
> -
> +#define virXendError(conn, codeval, fmt...)                                  \
> +    do {                                                                     \
> +        if (virGetLastError() == NULL) {                                     \
> +            virReportErrorHelper(conn, VIR_FROM_XEND, codeval, __FILE__,     \
> +                                 __FUNCTION__, __LINE__, fmt);               \
> +        }                                                                    \
> +    } while (0)
> + 
>  #define virXendErrorInt(conn, code, ival)                                    \
> -        virXendError(conn, code, "%d", ival)
> +    virXendError(conn, code, "%d", ival)

I don't like this change - we are trying to remove all driver specific
error reporting macros. If some part of the Xen driver is overwriting 
an existing error message during the failure path, either that is 
explicit, or a mistake that needs to be fixed.

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