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

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.

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; }

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; - }

/* 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;

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; }

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