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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org