[libvirt] [PATCH 04/24] maint: improve error condition style in public API

[John Ferlan]

On 12/28/2013 11:11 AM, Eric Blake wrote:
> While auditing error messages in libvirt.c, I found a couple
> instances that had not been converted to modern error styles,
> and a few places that failed to dispatch the error through
> the known-good connection.
> 
> * src/libvirt.c (virDomainPinEmulator, virDomainGetDiskErrors)
> (virDomainSendKey, virDomainGetSecurityLabelList)
> (virDomainGetEmulatorPinInfo): Use typical error reporting.
> (virConnectGetCPUModelNames, virConnectRegisterCloseCallback)
> (virConnectUnregisterCloseCallback, virDomainGetUUID): Report
> error through connection.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/libvirt.c | 54 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
> 
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 815dd64..33f7078 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -3588,11 +3588,15 @@ virDomainGetUUID(virDomainPtr domain, unsigned char *uuid)
>          virDispatchError(NULL);
>          return -1;
>      }
> -    virCheckNonNullArgReturn(uuid, -1);
> +    virCheckNonNullArgGoto(uuid, error);
> 
>      memcpy(uuid, &domain->uuid[0], VIR_UUID_BUFLEN);
> 
>      return 0;
> +
> +error:
> +    virDispatchError(domain->conn);
> +    return -1;
>  }
> 
> 
> @@ -9866,11 +9870,14 @@ virDomainSendKey(virDomainPtr domain,
> 
>      virResetLastError();
> 
> -    if (keycodes == NULL ||
> -        nkeycodes <= 0 || nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
> -        virLibDomainError(VIR_ERR_OPERATION_INVALID, __FUNCTION__);
> -        virDispatchError(NULL);
> -        return -1;
> +    virCheckNonNullArgGoto(keycodes, error);
> +    virCheckPositiveArgGoto(nkeycodes, error);
> +
> +    if (nkeycodes > VIR_DOMAIN_SEND_KEY_MAX_KEYS) {
> +        virReportInvalidArg(nkeycodes,
> +                            _("nkeycodes in %s must be less than %d"),

technically...

less than or equal to ...


> +                            __FUNCTION__, VIR_DOMAIN_SEND_KEY_MAX_KEYS);
> +        goto error;
>      }

I think the above code may need to move after the next two domain checks
(although looking forward, I realize it may be irrelevant)...

If domain is NULL at the goto error above, then the domain->conn deref
in error: will cause more havoc.

> 
>      if (!VIR_IS_CONNECTED_DOMAIN(domain)) {
> @@ -10477,10 +10484,8 @@ virDomainPinEmulator(virDomainPtr domain, unsigned char *cpumap,
>          goto error;
>      }
> 
> -    if ((cpumap == NULL) || (maplen < 1)) {
> -        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> -        goto error;
> -    }
> +    virCheckNonNullArgGoto(cpumap, error);
> +    virCheckPositiveArgGoto(maplen, error);
> 
>      conn = domain->conn;
> 
> @@ -10537,15 +10542,15 @@ virDomainGetEmulatorPinInfo(virDomainPtr domain, unsigned char *cpumap,
>          return -1;
>      }
> 
> -    if (!cpumap || maplen <= 0) {
> -        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> -        goto error;
> -    }
> +    virCheckNonNullArgGoto(cpumap, error);
> +    virCheckPositiveArgGoto(maplen, error);
> 
>      /* At most one of these two flags should be set.  */
>      if ((flags & VIR_DOMAIN_AFFECT_LIVE) &&
>          (flags & VIR_DOMAIN_AFFECT_CONFIG)) {
> -        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        virReportInvalidArg(flags,
> +                            _("flags 'affect live' and 'affect config' in %s are mutually exclusive"),
> +                            __FUNCTION__);

The new error message is longer than 80 columns.. split across multiple
lines.

ACK with nits.

John

>          goto error;
>      }
>      conn = domain->conn;
> @@ -10759,10 +10764,7 @@ virDomainGetSecurityLabelList(virDomainPtr domain,
>          return -1;
>      }
> 
> -    if (seclabels == NULL) {
> -        virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> -        goto error;
> -    }
> +    virCheckNonNullArgGoto(seclabels, error);
> 
>      conn = domain->conn;
> 
> @@ -18815,7 +18817,7 @@ virConnectGetCPUModelNames(virConnectPtr conn, const char *arch, char ***models,
>          virDispatchError(NULL);
>          return -1;
>      }
> -    virCheckNonNullArgReturn(arch, -1);
> +    virCheckNonNullArgGoto(arch, error);
> 
>      if (conn->driver->connectGetCPUModelNames) {
>          int ret;
> @@ -21887,10 +21889,10 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>      return 0;
> 
>  error:
> +    virDispatchError(conn);
>      virObjectUnlock(conn->closeCallback);
>      virMutexUnlock(&conn->lock);
>      virObjectUnref(conn);
> -    virDispatchError(NULL);
>      return -1;
>  }
> 
> @@ -21945,9 +21947,9 @@ virConnectUnregisterCloseCallback(virConnectPtr conn,
>      return 0;
> 
>  error:
> +    virDispatchError(conn);
>      virObjectUnlock(conn->closeCallback);
>      virMutexUnlock(&conn->lock);
> -    virDispatchError(NULL);
>      return -1;
>  }
> 
> @@ -22305,10 +22307,10 @@ virDomainGetDiskErrors(virDomainPtr dom,
>          return -1;
>      }
> 
> -    if ((!errors && maxerrors) || (errors && !maxerrors)) {
> -        virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__);
> -        goto error;
> -    }
> +    if (maxerrors)
> +        virCheckNonNullArgGoto(errors, error);
> +    else
> +        virCheckNullArgGoto(errors, error);
> 
>      if (dom->conn->driver->domainGetDiskErrors) {
>          int ret = dom->conn->driver->domainGetDiskErrors(dom, errors,
>