[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] errors: Improve error reporting to log multiple errors instead of just the last one



Oops, I'm sorry as this didn't pass the syntax check.

To pass the syntax check the new add-on patch is necessary:

diff --git a/src/util/virterror.c b/src/util/virterror.c
index b03ae7c..55269b9 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -198,8 +198,7 @@ virLastErrFreeData(void *data)
         virResetError(errs->lastError);
         VIR_FREE(errs->lastError);
     }
-    if (errs->errors)
-        VIR_FREE(errs->errors);
+    VIR_FREE(errs->errors);
     VIR_FREE(errs);
 }

With this applied on top of my patch it's working fine to pass all of
the tests (make check) and also the syntax check.

Also, when trying to debug the tests the gnulib tests are really
annoying. Would somebody consider creating some alias like `make
vircheck` to just do the libvirt tests, i.e. with bypassing the gnulib
tests. If you debug some test then it's pretty annoying to run through
the gnulib tests everytime you invoke `make check`.

Michal

On 01/19/2012 02:13 PM, Michal Novotny wrote:
> This patch introduces a new structure called virErrorsPtr which can get all
> the errors that occurred since the connection open. The error callback function
> is being used as many times as necessary. The new public function called
> virGetAllErrors() has been introduced to get all the errors that occurred.
>
> Also, a new test called errorreport has been written to test the functionality
> of error logging for multiple error occurrences.
>
> Signed-off-by: Michal Novotny <minovotn redhat com>
> ---
>  include/libvirt/virterror.h   |   14 ++++++
>  python/generator.py           |    1 +
>  src/util/virterror.c          |   99 ++++++++++++++++++++++++++++++++++++----
>  src/util/virterror_internal.h |    1 +
>  tests/Makefile.am             |    9 +++-
>  tests/errorreport.c           |   74 ++++++++++++++++++++++++++++++
>  6 files changed, 186 insertions(+), 12 deletions(-)
>  create mode 100644 tests/errorreport.c
>
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index e896d67..c7a8018 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -118,6 +118,19 @@ struct _virError {
>                                           see note above */
>  };
>  
> +/*
> + * virErrors:
> + *
> + * A libvirt Errors array instance
> + */
> +typedef struct _virErrors virErrors;
> +typedef virErrors *virErrorsPtr;
> +struct _virErrors {
> +    virErrorPtr lastError;
> +    unsigned int nerrors;
> +    virErrorPtr errors;
> +};
> +
>  /**
>   * virErrorNumber:
>   *
> @@ -261,6 +274,7 @@ typedef void (*virErrorFunc) (void *userData, virErrorPtr error);
>   */
>  
>  virErrorPtr		virGetLastError		(void);
> +virErrorsPtr		virGetAllErrors		(void);
>  virErrorPtr		virSaveLastError	(void);
>  void			virResetLastError	(void);
>  void			virResetError		(virErrorPtr err);
> diff --git a/python/generator.py b/python/generator.py
> index 6fee3a4..315eb51 100755
> --- a/python/generator.py
> +++ b/python/generator.py
> @@ -356,6 +356,7 @@ skip_impl = (
>      'virDomainSnapshotListChildrenNames',
>      'virConnGetLastError',
>      'virGetLastError',
> +    'virGetAllErrors',
>      'virDomainGetInfo',
>      'virDomainGetState',
>      'virDomainGetControlInfo',
> diff --git a/src/util/virterror.c b/src/util/virterror.c
> index 380dc56..b03ae7c 100644
> --- a/src/util/virterror.c
> +++ b/src/util/virterror.c
> @@ -190,11 +190,17 @@ static const char *virErrorDomainName(virErrorDomain domain) {
>  static void
>  virLastErrFreeData(void *data)
>  {
> -    virErrorPtr err = data;
> -    if (!err)
> +    virErrorsPtr errs = data;
> +    if (!errs)
>          return;
> -    virResetError(err);
> -    VIR_FREE(err);
> +
> +    if (errs->lastError) {
> +        virResetError(errs->lastError);
> +        VIR_FREE(errs->lastError);
> +    }
> +    if (errs->errors)
> +        VIR_FREE(errs->errors);
> +    VIR_FREE(errs);
>  }
>  
>  
> @@ -262,14 +268,18 @@ virCopyError(virErrorPtr from,
>  static virErrorPtr
>  virLastErrorObject(void)
>  {
> -    virErrorPtr err;
> -    err = virThreadLocalGet(&virLastErr);
> -    if (!err) {
> -        if (VIR_ALLOC(err) < 0)
> +    virErrorsPtr errs = NULL;
> +
> +    errs = virThreadLocalGet(&virLastErr);
> +    if (!errs) {
> +        if (VIR_ALLOC(errs) < 0)
> +            return NULL;
> +        if (VIR_ALLOC(errs->lastError) < 0)
>              return NULL;
> -        virThreadLocalSet(&virLastErr, err);
> +        virThreadLocalSet(&virLastErr, errs);
>      }
> -    return err;
> +
> +    return errs->lastError;
>  }
>  
>  
> @@ -292,6 +302,27 @@ virGetLastError(void)
>      return err;
>  }
>  
> +/*
> + * virGetAllErrors:
> + *
> + * Provide a pointer to all errors caught at the library level
> + *
> + * The error object is kept in thread local storage, so separate
> + * threads can safely access this concurrently.
> + *
> + * Returns a pointer to all errors caught or NULL if none occurred.
> + */
> +virErrorsPtr
> +virGetAllErrors(void)
> +{
> +    virErrorsPtr errs;
> +
> +    errs = virThreadLocalGet(&virLastErr);
> +    if (!errs || errs->nerrors == 0)
> +        return NULL;
> +    return errs;
> +}
> +
>  /**
>   * virSetError:
>   * @newerr: previously saved error object
> @@ -316,11 +347,56 @@ virSetError(virErrorPtr newerr)
>  
>      virResetError(err);
>      ret = virCopyError(newerr, err);
> +
> +    /* Add error into the error array */
> +    virAddError(newerr);
>  cleanup:
>      errno = saved_errno;
>      return ret;
>  }
>  
> +/*
> + * virAddError:
> + * @err: error pointer
> + *
> + * Add the error to the array of error pointer
> + */
> +void
> +virAddError(virErrorPtr err)
> +{
> +    virErrorsPtr errs;
> +
> +    /* Discard all error codes that mean 'no error' */
> +    if (!err || err->code == VIR_ERR_OK)
> +        return;
> +
> +    errs = virThreadLocalGet(&virLastErr);
> +
> +    /* Shouldn't happen but doesn't hurt to check */
> +    if (!errs)
> +        return;
> +
> +    if (errs->errors == NULL) {
> +        if (VIR_ALLOC(errs->errors) < 0)
> +            return;
> +        errs->nerrors = 0;
> +    }
> +    else {
> +        if (VIR_REALLOC_N(errs->errors, errs->nerrors + 1) < 0)
> +            return;
> +    }
> +
> +    /* Insert data into errors array element */
> +    errs->errors[errs->nerrors].domain = err->domain;
> +    errs->errors[errs->nerrors].code = err->code;
> +    errs->errors[errs->nerrors].message = strdup(err->message);
> +    errs->errors[errs->nerrors].level = err->level;
> +    errs->errors[errs->nerrors].int1 = err->int1;
> +    errs->errors[errs->nerrors].int2 = err->int2;
> +
> +    errs->nerrors++;
> +}
> +
>  /**
>   * virCopyLastError:
>   * @to: target to receive the copy
> @@ -730,6 +806,9 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
>      to->int1 = int1;
>      to->int2 = int2;
>  
> +    /* Add error into the error array */
> +    virAddError(to);
> +
>      /*
>       * Hook up the error or warning to the logging facility
>       * XXXX should we include filename as 'category' instead of domain name ?
> diff --git a/src/util/virterror_internal.h b/src/util/virterror_internal.h
> index d61ea0d..b26aec2 100644
> --- a/src/util/virterror_internal.h
> +++ b/src/util/virterror_internal.h
> @@ -86,6 +86,7 @@ void virReportOOMErrorFull(int domcode,
>  
>  
>  int virSetError(virErrorPtr newerr);
> +void virAddError(virErrorPtr err);
>  void virDispatchError(virConnectPtr conn);
>  const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen);
>  
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index f3b0c09..f15c915 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -97,7 +97,7 @@ check_PROGRAMS = virshtest conftest sockettest \
>  	commandtest commandhelper seclabeltest \
>  	hashtest virnetmessagetest virnetsockettest ssh \
>  	utiltest virnettlscontexttest shunloadtest \
> -	virtimetest
> +	virtimetest errorreport
>  
>  check_LTLIBRARIES = libshunload.la
>  
> @@ -221,6 +221,7 @@ TESTS = virshtest \
>  	virtimetest \
>  	shunloadtest \
>  	utiltest \
> +	errorreport \
>  	$(test_scripts)
>  
>  if HAVE_YAJL
> @@ -321,6 +322,10 @@ xencapstest_SOURCES = \
>  	xencapstest.c testutils.h testutils.c
>  xencapstest_LDADD = ../src/libvirt_driver_xen.la $(LDADDS)
>  
> +errorreport_SOURCES = \
> +	errorreport.c testutils.h testutils.c
> +errorreport_LDADD = $(LDADDS)
> +
>  reconnect_SOURCES = \
>  	reconnect.c testutils.h testutils.c
>  reconnect_LDADD = $(LDADDS)
> @@ -331,7 +336,7 @@ statstest_LDADD = ../src/libvirt_driver_xen.la $(LDADDS)
>  
>  else
>  EXTRA_DIST += xml2sexprtest.c sexpr2xmltest.c xmconfigtest.c \
> -	xencapstest.c reconnect.c \
> +	xencapstest.c reconnect.c errorreport.c \
>  	testutilsxen.c testutilsxen.h
>  endif
>  
> diff --git a/tests/errorreport.c b/tests/errorreport.c
> new file mode 100644
> index 0000000..cbe84dd
> --- /dev/null
> +++ b/tests/errorreport.c
> @@ -0,0 +1,74 @@
> +#define BOGUS_NAME "SomeBogusDomainName"
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include "internal.h"
> +#include "testutils.h"
> +#include "command.h"
> +
> +/*
> + * malloc/realloc function used in this code since I was unable to use
> + * VIR_[RE]ALLOC[_N] functions as the compilation failed
> + */
> +
> +int numErrors = 0;
> +int *myErrorCodes = NULL;
> +
> +static void errorHandler(void *userData ATTRIBUTE_UNUSED,
> +                         virErrorPtr error) {
> +    myErrorCodes = (int *)realloc( myErrorCodes, (numErrors + 1) * sizeof(int));
> +
> +    myErrorCodes[numErrors] = error->code;
> +    numErrors++;
> +}
> +
> +static int
> +mymain(void)
> +{
> +    int i, ret = EXIT_SUCCESS;
> +    virConnectPtr conn;
> +    virDomainPtr dom;
> +    virErrorsPtr errs;
> +
> +    myErrorCodes = (int *)malloc( sizeof(int) );
> +
> +    virSetErrorFunc(NULL, errorHandler);
> +
> +    conn = virConnectOpen(NULL);
> +    if (conn == NULL)
> +        conn = virConnectOpenReadOnly(NULL);
> +
> +    /* Attempt to fail for the first time */
> +    dom = virDomainLookupByID(conn, -1);
> +    /* Attempt for the second time */
> +    virDomainFree(dom);
> +    /* ... third time ... */
> +    dom = virDomainLookupByName(conn, BOGUS_NAME);
> +    /* Now close the connection */
> +    virConnectClose(conn);
> +
> +    /* ...and try to lookup - generate error for the fourth time :-) */
> +    dom = virDomainLookupByName(conn, BOGUS_NAME);
> +
> +    errs = virGetAllErrors();
> +    if (errs) {
> +        if (errs->nerrors != numErrors) {
> +            ret = EXIT_FAILURE;
> +            goto cleanup;
> +        }
> +
> +        for (i = 0; i < errs->nerrors; i++) {
> +            if (errs->errors[i].code != myErrorCodes[i])
> +                fprintf(stderr, "Error code for error #%d mismatch: expect %d, actual %d\n", i, myErrorCodes[i], errs->errors[i].code);
> +        }
> +    }
> +
> +cleanup:
> +    free(myErrorCodes);
> +
> +    return ret;
> +}
> +
> +VIRT_TEST_MAIN(mymain)


-- 
Michal Novotny <minovotn redhat com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]