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

[Marc Hartmayer]

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.

But I can understand, there is surely someone who might think "hey, the
loop is not running yet, the call was successful => I can still use the
data".

>
> I would think a little loss of memory is better than using memory you
> didn't expect to be free'd when virConnectRegisterCloseCallback was
> successful.

Yes, definitely. But a memory leak is still a memory leak… So I'd like
it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an
hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we
would change this (as I suggested in v1), at least our remote_driver
would be fenced against a memory leak (even without this patch). But
this would be an API change :(

doRemoteOpen:
    …
    priv->serverCloseCallback =  remoteConnectSupportsFeatureUnlocked(conn,
                                    priv,
                                    VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
    if (!priv->serverCloseCallback) {
        VIR_INFO("Close callback registering isn't supported "
        "by the remote side.");
    }
    …

Thanks for taking a look!

>
> John
>
>>
>>      return 0;
>>
>>
>
--
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