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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list