[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 <ramyelkest at gmail.com>
> ---
>  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 @@ void                    virResetLastError       (void);
>  void                    virResetError           (virErrorPtr err);
>  void                    virFreeError            (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