[...]
+ callback->program = virObjectRef(remoteProgram); + /* eventID, callbackID, and legacy are not used */ + callback->eventID = -1; + callback->callbackID = -1; if (virConnectRegisterCloseCallback(priv->conn, remoteRelayConnectionClosedEvent, - client, NULL) < 0) + callback, remoteEventCallbackFree) < 0) goto cleanup;
@callback would be leaked in the normal path...
By normal path, you mean without the first patch?
I was following how the other register functions proceeded and they saved the callback in a list to be freed at unregister. So if Register succeeds, rv == 0 and the removeEventCallbackFree(callback) isn't called and we leak callback. At least that's how I read it
First - sorry for the late response!
The unregister/free’ing is either done within the 'remoteClientFreePrivateCallbacks' call or by an explicit call of 'remoteDispatchConnectUnregisterCloseCallback'. Do we agree on this? If yes: the function 'remoteClientFreePrivateCallbacks' calls the virConnectUnregisterCloseCallback -> remoteEventCallbackFree => no memory leak.
There will be only a memory leak if the virConnectRegisterCloseCallback call succeeds but the used driver had no support for registering a close callback (conn->driver->connectRegisterCloseCallback was NULL) AND the first patch of this series were not applied.
Did I miss something?
Trying to recollect where I was.... and I cannot... I probably was flipping between patched and unpatched code. I assume it had something to do with adding the callback to a list, running DEREG_CB processing, and perhaps seeing DELETE_ELEMENT that got me overthinking, but taking a second look now I don't believe there's an issue. So, to make it official then... Reviewed-by: John Ferlan <jferlan@redhat.com> John