[libvirt] [PATCH libvirt v2 1/9] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback

[Marc Hartmayer]

On Fri, Apr 27, 2018 at 02:16 AM +0200, John Ferlan <jferlan at redhat.com> wrote:
> On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
>> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>>> On 04/12/2018 08:40 AM, Marc Hartmayer wrote:
>>>> The commit 'close callback: move it to driver' (88f09b75eb99) moved
>>>> the responsibility for the close callback to the driver. But if the
>>>> driver doesn't support the connectRegisterCloseCallback API this
>>>> function does nothing, even no unsupported error report. This behavior
>>>> may lead to problems, for example memory leaks, as the caller cannot
>>>> differentiate whether the close callback was 'really' registered or
>>>> not.
>>>>
>>>> Therefore, call directly @freecb if the connectRegisterCloseCallback
>>>> API is not supported by the driver used by the connection.
>>>>
>>>> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
>>>> ---
>>>>  src/libvirt-host.c | 10 +++++++---
>>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>>>> index 7ff7407a0874..cb2ace7d9778 100644
>>>> --- a/src/libvirt-host.c
>>>> +++ b/src/libvirt-host.c
>>>> @@ -1221,9 +1221,13 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
>>>>      virCheckConnectReturn(conn, -1);
>>>>      virCheckNonNullArgGoto(cb, error);
>>>>
>>>> -    if (conn->driver->connectRegisterCloseCallback &&
>>>> -        conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0)
>>>> -        goto error;
>>>> +    if (conn->driver->connectRegisterCloseCallback) {
>>>> +        if (conn->driver->connectRegisterCloseCallback(conn, cb, opaque, freecb) < 0)
>>>> +            goto error;
>>>> +    } else {
>>>> +        if (freecb)
>>>> +            freecb(opaque);
>>>> +    }
>>>
>>> I see this follows what Daniel suggests from v1:
>>>
>>> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>>>
>>> but I guess it still baffles me about calling @freecb w/ @opaque. If
>>> there was a @freecb routine supplied it'd be called and free @opaque
>>> which may actually be used afterwards - after all this is a
>>> RegisterCloseCallback which would conceptually be called after open, but
>>> before perhaps using the @opaque.
>> 
>> I understand your concerns.
>> 
>>> E.G., the virsh code uses this - with
>>> virshReconnect being called from virshInit before entering the loop
>>> processing commands. So if @ctl was free'd - things wouldn't be good!
>>> Running in debug using '-c test:///default' dumps you into the else.
>> 
>> virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
>> registered callback has not the responsibility to free the memory.
>> 
>
> True virsh uses NULL so it's fine; however, I was thinking about more
> generically - why would a Register routine with a callback to free
> memory free the memory upon successful register.
>
> I'm still not sure I understand why the API cannot return a failure, but
> Daniel says it cannot.
>
> I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
> ping him in the morning...
>
> John

Polite ping.

[…snip]

-- 
Beste Grüße / Kind regards
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294