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