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