[libvirt] [PATCH libvirt v2 4/9] remote: Use domainClientEventCallbacks for remoteReplayConnectionClosedEvent

[John Ferlan]

[...]

>>>>
>>>>> +    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 at redhat.com>

John