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

2 Jan 2014

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@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,


    

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

John Ferlan