Re: [libvirt] [PATCH v2] Merge all returns paths from dispatcher into single path

15 Apr 2011

On 04/14/2011 07:28 AM, Daniel P. Berrange wrote:
...
The dispatcher functions have numerous places where they
return to the caller. This leads to duplicated cleanup
code, often resulting in memory leaks. It makes it harder
to ensure that errors are dispatched before freeing objects,
which may overwrite the original error.
Rather than review your entire patch again, I just reviewed this interdiff.

You really did get rid of all remoteDispatchOOMError callers; I'd be
fine if you squashed in deleting the declaration from
daemon/dispatch.[ch] as part of this patch, or as a followup.
...

diff --git c/daemon/remote.c w/daemon/remote.c
index 8f4d6a6..a25c095 100644
--- c/daemon/remote.c
+++ w/daemon/remote.c
@@ -912,7 +912,7 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
                                            remote_domain_get_scheduler_parameters_ret *ret)
 {
     virDomainPtr dom = NULL;
-    virSchedParameterPtr params;
+    virSchedParameterPtr params = NULL;
     int i, r, nparams;
     int rv = -1;
@@ -927,10 +927,8 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large"));
         goto cleanup;
     }
-    if (VIR_ALLOC_N(params, nparams) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
+    if (VIR_ALLOC_N(params, nparams) < 0)
+        goto no_memory;
dom = get_nonnull_domain(conn, args->dom);
     if (dom == NULL) {
@@ -1002,7 +1000,7 @@ remoteDispatchDomainSetSchedulerParameters(struct qemud_server *server ATTRIBUTE
 {
     virDomainPtr dom = NULL;
     int i, r, nparams;
-    virSchedParameterPtr params;
+    virSchedParameterPtr params = NULL;
     int rv = -1;
if (!conn) {
@@ -1960,7 +1958,7 @@ remoteDispatchDomainGetSecurityLabel(struct qemud_server *server ATTRIBUTE_UNUSE
                                      remote_domain_get_security_label_ret *ret)
 {
     virDomainPtr dom = NULL;
-    virSecurityLabelPtr seclabel;
+    virSecurityLabelPtr seclabel = NULL;
     int rv = -1;
if (!conn) {
@@ -3041,7 +3039,7 @@ remoteDispatchDomainSetMemoryParameters(struct qemud_server *server
 {
     virDomainPtr dom = NULL;
     int i, r, nparams;
-    virMemoryParameterPtr params;
+    virMemoryParameterPtr params = NULL;
     unsigned int flags;
     int rv = -1;
@@ -3142,7 +3140,7 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
                                         * ret)
 {
     virDomainPtr dom = NULL;
-    virMemoryParameterPtr params;
+    virMemoryParameterPtr params = NULL;
     int i, r, nparams;
     unsigned int flags;
     int rv = -1;
@@ -3233,8 +3231,6 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server
 success:
     rv = 0;
-no_memory:
-    virReportOOMError();
 cleanup:
     if (rv < 0) {
         remoteDispatchError(rerr);
@@ -3246,6 +3242,10 @@ cleanup:
         virDomainFree(dom);
     VIR_FREE(params);
     return rv;
+
+no_memory:
+    virReportOOMError();
+    goto cleanup;
 }
Good, this addresses my review comments.
...
static int
@@ -3262,7 +3262,7 @@ remoteDispatchDomainSetBlkioParameters(struct qemud_server *server
 {
     virDomainPtr dom = NULL;
     int i, r, nparams;
-    virBlkioParameterPtr params;
+    virBlkioParameterPtr params = NULL;
     unsigned int flags;
     int rv = -1;
@@ -3363,7 +3363,7 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server
                                         * ret)
 {
     virDomainPtr dom = NULL;
-    virBlkioParameterPtr params;
+    virBlkioParameterPtr params = NULL;
     int i, r, nparams;
     unsigned int flags;
     int rv = -1;
@@ -4846,9 +4846,8 @@ remoteDispatchAuthSaslInit(struct qemud_server *server,
                     virStrerror(errno, ebuf, sizeof ebuf));
         goto error;
     }
-    if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) {
+    if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL)
         goto error;
-    }
Looks like the cleanups from one of your followup patches leaked in here
when you reverted your sasl changes, but no real harm leaving this hunk in.
...
/* Get remote address in form  IPADDR:PORT */
     sa.len = sizeof(sa.data.stor);
@@ -4964,7 +4963,7 @@ authfail:
 error:
     PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_SASL);
     virMutexUnlock(&client->lock);
-    goto error;
+    return -1;
 }
@@ -5126,6 +5125,7 @@ remoteDispatchAuthSaslStart(struct qemud_server *server,
     if (serverout) {
         if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
             virReportOOMError();
+            remoteDispatchError(rerr);
             goto error;
         }
         memcpy(ret->data.data_val, serverout, serveroutlen);
@@ -5227,6 +5227,7 @@ remoteDispatchAuthSaslStep(struct qemud_server *server,
     if (serverout) {
         if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) {
             virReportOOMError();
+            remoteDispatchError(rerr);
             goto error;
         }
         memcpy(ret->data.data_val, serverout, serveroutlen);
@@ -5403,7 +5404,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
     client->auth = REMOTE_AUTH_NONE;
virMutexUnlock(&client->lock);
-    rv = 0;
+    return 0;
authfail:
     PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);
@@ -5535,7 +5536,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server,
     client->auth = REMOTE_AUTH_NONE;
virMutexUnlock(&client->lock);
-    rv = 0;
+    return 0;
Looks like you correctly reverted any changes to sasl code, as planned.
...
authfail:
     PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);
@@ -6964,7 +6965,7 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
                                   remote_node_device_get_parent_ret *ret)
 {
     virNodeDevicePtr dev = NULL;
-    const char *parent;
+    const char *parent = NULL;
     int rv = -1;
if (!conn) {
@@ -6988,8 +6989,8 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED,
             virReportOOMError();
             goto cleanup;
         }
-        *parent_p = strdup(parent);
-        if (*parent_p == NULL) {
+        if (!(*parent_p = strdup(parent))) {
+            VIR_FREE(parent_p);
             virReportOOMError();
             goto cleanup;
         }
For all my waffling reviews, I think you got this one right now.
...
@@ -8413,7 +8414,7 @@ remoteDispatchDomainSnapshotCreateXml(struct qemud_server *server ATTRIBUTE_UNUS
                                       remote_domain_snapshot_create_xml_args *args,
                                       remote_domain_snapshot_create_xml_ret *ret)
 {
-    virDomainSnapshotPtr snapshot;
+    virDomainSnapshotPtr snapshot = NULL;
     virDomainPtr domain = NULL;
     int rv = -1;
@@ -8589,7 +8590,7 @@ remoteDispatchDomainSnapshotLookupByName(struct qemud_server *server ATTRIBUTE_U
                                          remote_domain_snapshot_lookup_by_name_args *args,
                                          remote_domain_snapshot_lookup_by_name_ret *ret)
 {
-    virDomainSnapshotPtr snapshot;
+    virDomainSnapshotPtr snapshot = NULL;
     virDomainPtr domain = NULL;
     int rv = -1;
@@ -8671,7 +8672,7 @@ remoteDispatchDomainSnapshotCurrent(struct qemud_server *server ATTRIBUTE_UNUSED
                                     remote_domain_snapshot_current_args *args,
                                     remote_domain_snapshot_current_ret *ret)
 {
-    virDomainSnapshotPtr snapshot;
+    virDomainSnapshotPtr snapshot = NULL;
     virDomainPtr domain = NULL;
     int rv = -1;
ACK - you addressed all my findings from v1.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    