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

2012-01-19 Thread Michal Novotny 
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 minov...@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);
  */
 
 virErrorPtrvirGetLastError (void);
+virErrorsPtr   virGetAllErrors (void);
 virErrorPtrvirSaveLastError(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 */
+

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

2012-01-19 Thread Michal Novotny 
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 minov...@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);

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

2012-01-19 Thread Daniel P. Berrange 
On Thu, Jan 19, 2012 at 02:13:59PM +0100, 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.

This impl is effectively an unbounded memory leak, if you consider
that applications will keep the same virConnectPtr open more or
less forever.

In addition any libvirt API that raises multiple errors should be
considered broken, so I don't think we should have any such API
for querying multiple errors.

What is the situation that motivated this new API ?

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-01-19 Thread Michal Novotny 
On 01/19/2012 02:20 PM, Daniel P. Berrange wrote:
 On Thu, Jan 19, 2012 at 02:13:59PM +0100, 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.
 This impl is effectively an unbounded memory leak, if you consider
 that applications will keep the same virConnectPtr open more or
 less forever.

 In addition any libvirt API that raises multiple errors should be
 considered broken, so I don't think we should have any such API
 for querying multiple errors.

 What is the situation that motivated this new API ?

This is simple. When you have situation with e.g. disk or domain
creation. It fails but you don't know why. Sometimes the disk creation
may fail on insufficient permissions and sometimes there may be not
enough space etc... The same for domain creation.

Also, we've been discussing this with Peter and we've both agreed that
it would be a nice feature of libvirt to have some API like this. I'm
CCing Peter for his feedback on this (not the patch rather than the
reason of implementation) as I have to admit that this patch would like
some tweaking most likely but the issue here is not different as you're
asking whether we really need this.

Peter, could you please provide Daniel any other example why you would
like this implemented?

Thanks,
Michal

-- 
Michal Novotny minov...@redhat.com, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-01-19 Thread Daniel P. Berrange 
On Thu, Jan 19, 2012 at 02:25:55PM +0100, Michal Novotny wrote:
 On 01/19/2012 02:20 PM, Daniel P. Berrange wrote:
  On Thu, Jan 19, 2012 at 02:13:59PM +0100, 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.
  This impl is effectively an unbounded memory leak, if you consider
  that applications will keep the same virConnectPtr open more or
  less forever.
 
  In addition any libvirt API that raises multiple errors should be
  considered broken, so I don't think we should have any such API
  for querying multiple errors.
 
  What is the situation that motivated this new API ?
 
 This is simple. When you have situation with e.g. disk or domain
 creation. It fails but you don't know why. Sometimes the disk creation
 may fail on insufficient permissions and sometimes there may be not
 enough space etc... The same for domain creation.

But for any single API call into libvirt, there is only one
error that is relevant upon failure.  If you are making a
sequence of multiple API calls, you should check each one
for an error, before trying the next call. Not doing so is
simply an application bug

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-01-19 Thread Michal Novotny 
On 01/19/2012 02:28 PM, Daniel P. Berrange wrote:
 On Thu, Jan 19, 2012 at 02:25:55PM +0100, Michal Novotny wrote:
 On 01/19/2012 02:20 PM, Daniel P. Berrange wrote:
 On Thu, Jan 19, 2012 at 02:13:59PM +0100, 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.
 This impl is effectively an unbounded memory leak, if you consider
 that applications will keep the same virConnectPtr open more or
 less forever.

 In addition any libvirt API that raises multiple errors should be
 considered broken, so I don't think we should have any such API
 for querying multiple errors.

 What is the situation that motivated this new API ?
 This is simple. When you have situation with e.g. disk or domain
 creation. It fails but you don't know why. Sometimes the disk creation
 may fail on insufficient permissions and sometimes there may be not
 enough space etc... The same for domain creation.
 But for any single API call into libvirt, there is only one
 error that is relevant upon failure.  If you are making a
 sequence of multiple API calls, you should check each one
 for an error, before trying the next call. Not doing so is
 simply an application bug

 Daniel
I can see your point Daniel and not I don't recall what we were talking
about with Peter exactly. Maybe he can recall so I'll leave it to him
but the truth is that I've been struggling with the reason of failure
and finally just the logging using the LIBVIRT_DEBUG=1  when running the
daemon helped me and I've been stuck before using it since the error was
masked by something AFAIK.

Michal

-- 
Michal Novotny minov...@redhat.com, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list