6:23 p.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org