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

On 04/13/2011 12:12 PM, 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.

* daemon/remote.c: Centralize all cleanup paths * daemon/stream.c: s/remoteDispatchConnError/remoteDispatchError/ * daemon/dispatch.c, daemon/dispatch.h: Replace remoteDispatchConnError with remoteDispatchError removing unused virConnectPtr

+++ b/daemon/dispatch.c @@ -112,8 +112,7 @@ void remoteDispatchOOMError (remote_error *rerr) }

-void remoteDispatchConnError (remote_error *rerr, - virConnectPtr conn ATTRIBUTE_UNUSED) +void remoteDispatchError(remote_error *rerr)

Two cleanups for the price of one (rename function to nuke unused parameter, while adding goto cleanup); but I'm okay with lumping them in the same commit.

@@ -458,20 +467,25 @@ remoteDispatchSupportsFeature(struct qemud_server *server ATTRIBUTE_UNUSED, remote_error *rerr, remote_supports_feature_args *args, remote_supports_feature_ret *ret) { + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open"));

Is virNetError really the best name for the new helper macro, especially since VIR_FROM_THIS is VIR_FROM_REMOTE? Should it instead be named virRemoteError()?

@@ -500,11 +514,16 @@ remoteDispatchGetType(struct qemud_server *server ATTRIBUTE_UNUSED, */ ret->type = strdup(type); if (!ret->type) { - remoteDispatchOOMError(rerr); - return -1; + virReportOOMError(); + goto cleanup; }

- return 0; + rv = 0; + +cleanup: + if (rv < 0) + remoteDispatchError(rerr);

That works. I wonder if we can completely get rid of remoteDispatchOOMError?

@@ -837,51 +911,47 @@ remoteDispatchDomainGetSchedulerParameters(struct qemud_server *server ATTRIBUTE remote_domain_get_scheduler_parameters_args *args, remote_domain_get_scheduler_parameters_ret *ret) {

if (VIR_ALLOC_N(params, nparams) < 0) { - remoteDispatchOOMError(rerr); - return -1; + virReportOOMError(); + goto cleanup;

This could be goto no_memory, rather than open-coding the OOM call, since you already have that label for other reasons in this function.

@@ -1146,21 +1232,21 @@ remoteDispatchDomainBlockPeek(struct qemud_server *server ATTRIBUTE_UNUSED, remote_domain_block_peek_args *args, remote_domain_block_peek_ret *ret) {

if (virDomainBlockPeek(dom, path, offset, size, ret->buffer.buffer_val, flags) == -1) { - /* free(ret->buffer.buffer_val); - caller frees */ - remoteDispatchConnError(rerr, conn); - virDomainFree(dom); - return -1; + goto cleanup; } - virDomainFree(dom);

- return 0; + rv = 0; + +cleanup: + if (rv < 0) { + remoteDispatchError(rerr); + VIR_FREE(ret->buffer.buffer_val);

Can we also clean up the caller to quit freeing (as a separate patch), now that we guarantee the cleanup here? Should we have a 'make syntax-check' rule that catches unadorned use of free() instead of VIR_FREE?

@@ -1780,46 +1959,46 @@ remoteDispatchDomainGetSecurityLabel(struct qemud_server *server ATTRIBUTE_UNUSE remote_domain_get_security_label_args *args, remote_domain_get_security_label_ret *ret) { - virDomainPtr dom; + virDomainPtr dom = NULL; virSecurityLabelPtr seclabel; + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;

+cleanup: + if (rv < 0) + remoteDispatchError(rerr); + if (dom) + virDomainFree(dom); + VIR_FREE(seclabel);

Oops - freeing an uninitialized pointer. Init seclabel to NULL.

@@ -2730,37 +3039,37 @@ remoteDispatchDomainSetMemoryParameters(struct qemud_server *server remote_domain_set_memory_parameters_args * args, void *ret ATTRIBUTE_UNUSED) { - virDomainPtr dom; + virDomainPtr dom = NULL; int i, r, nparams; virMemoryParameterPtr params; unsigned int flags; + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;

+cleanup: + if (rv < 0) + remoteDispatchError(rerr); + VIR_FREE(params);

Oops - another free(uninitialized).

@@ -2830,41 +3141,37 @@ remoteDispatchDomainGetMemoryParameters(struct qemud_server *server remote_domain_get_memory_parameters_ret * ret) { - virDomainPtr dom; + virDomainPtr dom = NULL; virMemoryParameterPtr params; int i, r, nparams; unsigned int flags; + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;

And again, params must be NULL.

- success: - virDomainFree(dom); - VIR_FREE(params); - - return 0; +success: + rv = 0;

- oom: - remoteDispatchOOMError(rerr); - cleanup: - virDomainFree(dom); - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); +no_memory: + virReportOOMError();

Ouch. You didn't mean for success: to fall through to no_memory:, but to cleanup:. Sink the OOM reporting down, and then jump back to cleanup.

@@ -2951,37 +3260,37 @@ remoteDispatchDomainSetBlkioParameters(struct qemud_server *server remote_domain_set_blkio_parameters_args * args, void *ret ATTRIBUTE_UNUSED) { - virDomainPtr dom; + virDomainPtr dom = NULL; int i, r, nparams; virBlkioParameterPtr params;

Same problem as SetMemoryParameters.

@@ -3051,41 +3362,37 @@ remoteDispatchDomainGetBlkioParameters(struct qemud_server *server remote_domain_get_blkio_parameters_ret * ret) { - virDomainPtr dom; + virDomainPtr dom = NULL; virBlkioParameterPtr params; int i, r, nparams; unsigned int flags;

+ +no_memory: + virReportOOMError(); + goto cleanup; }

Same problem as GetMemoryParameters with params being initialized, but at least the no_memory label worked this time.

@@ -4287,7 +4814,7 @@ remoteDispatchAuthList(struct qemud_server *server, static int remoteDispatchAuthSaslInit(struct qemud_server *server, struct qemud_client *client, - virConnectPtr conn, + virConnectPtr conn ATTRIBUTE_UNUSED, remote_message_header *hdr ATTRIBUTE_UNUSED, remote_error *rerr, void *args ATTRIBUTE_UNUSED,

if ((localAddr = virSocketFormatAddrFull(&sa, true, ";")) == NULL) { - remoteDispatchConnError(rerr, conn); goto error;

Losing the error reporting here...

@@ -4439,7 +4964,7 @@ authfail: error: PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_SASL); virMutexUnlock(&client->lock); - return -1; + goto error;

...and replacing it with an infinite loop here. I don't think that's what you meant to do.

@@ -4600,7 +5125,7 @@ remoteDispatchAuthSaslStart(struct qemud_server *server, /* NB, distinction of NULL vs "" is *critical* in SASL */ if (serverout) { if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) { - remoteDispatchOOMError(rerr); + virReportOOMError(); goto error; }

This changes from remoteDispatchOOMError to virReportOOM, but the error: label doesn't call remoteDispatchError to actually dispatch it.

@@ -4701,7 +5226,7 @@ remoteDispatchAuthSaslStep(struct qemud_server *server, /* NB, distinction of NULL vs "" is *critical* in SASL */ if (serverout) { if (VIR_ALLOC_N(ret->data.data_val, serveroutlen) < 0) { - remoteDispatchOOMError(rerr); + virReportOOMError(); goto error; }

Likewise.

@@ -4878,7 +5403,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server, client->auth = REMOTE_AUTH_NONE;

virMutexUnlock(&client->lock); - return 0; + rv = 0;

authfail: PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);

Oops - that was unintended fallthrough.

@@ -5010,7 +5535,7 @@ remoteDispatchAuthPolkit(struct qemud_server *server, client->auth = REMOTE_AUTH_NONE;

virMutexUnlock(&client->lock); - return 0; + rv = 0;

authfail: PROBE(CLIENT_AUTH_FAIL, "fd=%d, auth=%d", client->fd, REMOTE_AUTH_POLKIT);

As was that.

@@ -6245,18 +6963,18 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED, remote_node_device_get_parent_args *args, remote_node_device_get_parent_ret *ret) { - virNodeDevicePtr dev; + virNodeDevicePtr dev = NULL; const char *parent; + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;

Oops, parent is uninitialized on this path...

}

dev = virNodeDeviceLookupByName(conn, args->name); if (dev == NULL) { - remoteDispatchConnError(rerr, conn); - return -1; + goto cleanup; }

parent = virNodeDeviceGetParent(dev);

...and malloc'd on this path.

@@ -6267,21 +6985,25 @@ remoteDispatchNodeDeviceGetParent(struct qemud_server *server ATTRIBUTE_UNUSED, /* remoteDispatchClientRequest will free this. */ char **parent_p; if (VIR_ALLOC(parent_p) < 0) { - virNodeDeviceFree(dev); - remoteDispatchOOMError(rerr); - return -1; + virReportOOMError(); + goto cleanup; } *parent_p = strdup(parent);

Rather than strdup()ing malloc's memory, this should be: *parent_p = parent; parent = NULL;

if (*parent_p == NULL) { - virNodeDeviceFree(dev); - remoteDispatchOOMError(rerr); - return -1; + virReportOOMError(); + goto cleanup; } ret->parent = parent_p; }

- virNodeDeviceFree(dev); - return 0; + rv = 0; + +cleanup: + if (rv < 0) + remoteDispatchError(rerr); + if (dev) + virNodeDeviceFree(dev);

...and add unconditional VIR_FREE(parent) here. I'm surprised we haven't noticed that leak before.

@@ -7784,32 +8672,36 @@ remoteDispatchDomainSnapshotCurrent(struct qemud_server *server ATTRIBUTE_UNUSED remote_domain_snapshot_current_ret *ret) { virDomainSnapshotPtr snapshot; - virDomainPtr domain; + virDomainPtr domain = NULL; + int rv = -1;

if (!conn) { - remoteDispatchFormatError(rerr, "%s", _("connection not open")); - return -1; + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup;

+cleanup: + if (rv < 0) + remoteDispatchError(rerr); + if (snapshot) + virDomainSnapshotFree(snapshot);

Oops, freeing uninitialized variable. All the rest look fine. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org