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

[Daniel P. Berrange]

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

It isn't malloc'd here actually. This is returning
a 'const char *'...

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

..meaning it isn't actually a leak here :-)


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