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. > > @@ -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?

> - 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.

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