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

[Eric Blake]

On 04/14/2011 06:43 AM, Daniel P. Berrange wrote:
>>> +        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

Fair enough.

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

What other files?  daemon/dispatch.h declares it and daemon/dispatch.c
implements it, but no one but daemon/remote.c uses it.

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

My question was about the commented-out /* free() - caller frees */
above.  But if you're correct, the function caller does _not_ free
ret->buffer.buffer_val on a -1 return, so there's nothing to fix in the
caller and this really does plug a leak.  On the other hand, calling
VIR_FREE on the error path is perfectly fine, even if the caller did
free that variable, because the caller will see NULL and be a no-op - so
my question was whether the caller is doing the free even for a -1
return which can now be cleaned up.

At any rate, I don't think you need to make any changes to this hunk,
and if you're right, we are plugging a leak rather than leaving a no-op
free in some caller.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110414/5e68627c/attachment-0001.sig>