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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list