Re: [libvirt] [PATCH 1/2] util: adding virHasLastError and virGetLastErrorCode/Domain

[Erik Skultety]

On Fri, May 04, 2018 at 04:46:47AM +0100, ramyelkest wrote:
> Many places in the code call virGetLastError() just to check the
> raised error code, or domain. However virGetLastError() can return
> NULL, so the code has to check for that as well.

s/as well/first.

>
> So Instead we create functions virGetLastErrorCode and virGetLastErrorDomain
> (in addition to the existing virGetLastErrorMessage) that always return a
> valid error code/domain/message, to simplify callers.

No paragraph, how about "This patch therefore introduces virGetLasErrorCode
function which always returns a valid error code, thus dropping the need to
perform any checks on the error object".

>
> Also created virHasLastErrorCode for completion
>
> My first commit, for:
> https://wiki.libvirt.org/page/BiteSizedTasks#Add_and_use_virGetLastErrorCode.28.29
>

You mentioned this in the cover letter already, no need to have it in the commit
messages too ;).

> Signed-off-by: Ramy Elkest 
> ---
>  include/libvirt/virterror.h |  3 +++
>  src/libvirt_public.syms |  7 +
>  src/util/virerror.c | 63 
> +
>  3 files changed, 73 insertions(+)
>
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index 3e7c7a0..8336258 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -344,6 +344,9 @@ voidvirResetLastError   (void);
>  voidvirResetError   (virErrorPtr err);
>  voidvirFreeError(virErrorPtr err);
>
> +int virHasLastError (void);
> +int virGetLastErrorCode (void);
> +int virGetLastErrorDomain   (void);
>  const char *virGetLastErrorMessage  (void);
>
>  virErrorPtr virConnGetLastError (virConnectPtr conn);
> diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
> index 95df3a0..3a641a3 100644
> --- a/src/libvirt_public.syms
> +++ b/src/libvirt_public.syms
> @@ -785,4 +785,11 @@ LIBVIRT_4.1.0 {
>  virStoragePoolLookupByTargetPath;
>  } LIBVIRT_3.9.0;
>
> +LIBVIRT_4.4.0 {
> +global:
> +virHasLastError;
> +virGetLastErrorCode;
> +virGetLastErrorDomain;
> +} LIBVIRT_4.1.0;
> +
>  #  define new API here using predicted next version number 
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index c000b00..818af2e 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -272,6 +272,69 @@ virGetLastError(void)
>
>
>  /**
> + * virHasLastError:
> + *
> + * Check for a reported last error
> + *
> + * The error object is kept in thread local storage, so separate
> + * threads can safely access this concurrently.
> + *
> + * Returns 1 if there is an error and the error code is not VIR_ERR_OK,
> +   otherwise returns 0
> + */
> +int
> +virHasLastError(void)
> +{
> +virErrorPtr err = virLastErrorObject();
> +if (!err || err->code == VIR_ERR_OK)
> +return 0;
> +return 1;
> +}

As you noted in the cover letter, ^this one is essentially identical to
virGetLastErrorCode, minus the "1 vs err->code" and since VIR_ERR_OK == 0, you
can use virGetLastErrorCode the same way as you used virHasLastError as a
replacement in many occurrences of virGetLastError(), so I don't see the need
for this function.

> +
> +
> +/**
> + * virGetLastErrorCode:
> + *
> + * Get the most recent error code
> + *
> + * The error object is kept in thread local storage, so separate
> + * threads can safely access this concurrently.
> + *
> + * Returns the most recent error code in this thread,
> + * or VIR_ERR_OK if none is set
> + */
> +int
> +virGetLastErrorCode(void)
> +{
> +virErrorPtr err = virLastErrorObject();
> +if (!err)
> +return VIR_ERR_OK;
> +return err->code;
> +}
> +
> +
> +/**
> + * virGetLastErrorDomain:
> + *
> + * Get the most recent error domain
> + *
> + * The error object is kept in thread local storage, so separate
> + * threads can safely access this concurrently.
> + *
> + * Returns the most recent error code in this thread,
> + * or VIR_FROM_NONE if none is set
> + */
> +int
> +virGetLastErrorDomain(void)
> +{
> +virErrorPtr err = virLastErrorObject();
> +if (!err)
> +return VIR_FROM_NONE;
> +return err->domain;
> +}

I can see a need for ^this function, even though we don't have an immediate use
case for it internally, it's still a public API which someone else might
consume, otherwise they'd have to query the object itself.

Erik

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

[libvirt] [PATCH 1/2] util: adding virHasLastError and virGetLastErrorCode/Domain

[ramyelkest]

Many places in the code call virGetLastError() just to check the
raised error code, or domain. However virGetLastError() can return
NULL, so the code has to check for that as well.

So Instead we create functions virGetLastErrorCode and virGetLastErrorDomain
(in addition to the existing virGetLastErrorMessage) that always return a
valid error code/domain/message, to simplify callers.

Also created virHasLastErrorCode for completion

My first commit, for:
https://wiki.libvirt.org/page/BiteSizedTasks#Add_and_use_virGetLastErrorCode.28.29

Signed-off-by: Ramy Elkest 
---
 include/libvirt/virterror.h |  3 +++
 src/libvirt_public.syms |  7 +
 src/util/virerror.c | 63 +
 3 files changed, 73 insertions(+)

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 3e7c7a0..8336258 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -344,6 +344,9 @@ voidvirResetLastError   (void);
 voidvirResetError   (virErrorPtr err);
 voidvirFreeError(virErrorPtr err);
 
+int virHasLastError (void);
+int virGetLastErrorCode (void);
+int virGetLastErrorDomain   (void);
 const char *virGetLastErrorMessage  (void);
 
 virErrorPtr virConnGetLastError (virConnectPtr conn);
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index 95df3a0..3a641a3 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -785,4 +785,11 @@ LIBVIRT_4.1.0 {
 virStoragePoolLookupByTargetPath;
 } LIBVIRT_3.9.0;
 
+LIBVIRT_4.4.0 {
+global:
+virHasLastError;
+virGetLastErrorCode;
+virGetLastErrorDomain;
+} LIBVIRT_4.1.0;
+
 #  define new API here using predicted next version number 
diff --git a/src/util/virerror.c b/src/util/virerror.c
index c000b00..818af2e 100644
--- a/src/util/virerror.c
+++ b/src/util/virerror.c
@@ -272,6 +272,69 @@ virGetLastError(void)
 
 
 /**
+ * virHasLastError:
+ *
+ * Check for a reported last error
+ *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
+ *
+ * Returns 1 if there is an error and the error code is not VIR_ERR_OK,
+   otherwise returns 0
+ */
+int
+virHasLastError(void)
+{
+virErrorPtr err = virLastErrorObject();
+if (!err || err->code == VIR_ERR_OK)
+return 0;
+return 1;
+}
+
+
+/**
+ * virGetLastErrorCode:
+ *
+ * Get the most recent error code
+ *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
+ *
+ * Returns the most recent error code in this thread,
+ * or VIR_ERR_OK if none is set
+ */
+int
+virGetLastErrorCode(void)
+{
+virErrorPtr err = virLastErrorObject();
+if (!err)
+return VIR_ERR_OK;
+return err->code;
+}
+
+
+/**
+ * virGetLastErrorDomain:
+ *
+ * Get the most recent error domain
+ *
+ * The error object is kept in thread local storage, so separate
+ * threads can safely access this concurrently.
+ *
+ * Returns the most recent error code in this thread,
+ * or VIR_FROM_NONE if none is set
+ */
+int
+virGetLastErrorDomain(void)
+{
+virErrorPtr err = virLastErrorObject();
+if (!err)
+return VIR_FROM_NONE;
+return err->domain;
+}
+
+
+/**
  * virGetLastErrorMessage:
  *
  * Get the most recent error message
-- 
2.7.4

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