On Wed, Apr 13, 2011 at 05:23:41PM -0600, Eric Blake wrote:
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()?
virNetError() is what my RPC series of patches uses throughout. Also it will change VIR_FROM_REMOTE to VIR_FROM_RPC
@@ -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?
Other files still use it, but it will be gone with the RPC rewrite.
@@ -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.
Opps, yes missed one.
@@ -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?
I'm not sure what you mean here ? If rv == 0, then 'ret' is expected to have the data present, and the caller will use xdr_free() to release it. If rv == -1, then 'ret' is expected to have not been initialized, hence this code must free that buffer.
- 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.
Yes, nice bug :-)
@@ -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.
I didn't mean to touch any of these SASL functions, since they're rather special. Will remove those chunks.
@@ -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.
Yes, will do Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|