[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]