[libvirt] [PATCH v3 2/6] virConnectRegisterCloseCallback: Cleanup 'opaque' if there is no connectRegisterCloseCallback

[Marc Hartmayer]

On Fri, Nov 08, 2019 at 04:52 PM +0100, Pavel Hrdina <phrdina at redhat.com> wrote:
> On Fri, Nov 01, 2019 at 06:35:44PM +0100, 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.ibm.com>
>> ---
>>  src/libvirt-host.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>> index 221a1b7a4353..9c3a19f33b12 100644
>> --- a/src/libvirt-host.c
>> +++ b/src/libvirt-host.c
>> @@ -1398,7 +1398,7 @@ virConnectIsAlive(virConnectPtr conn)
>>   * @conn: pointer to connection object
>>   * @cb: callback to invoke upon close
>>   * @opaque: user data to pass to @cb
>> - * @freecb: callback to free @opaque
>> + * @freecb: callback to free @opaque when not used anymore
>>   *
>>   * Registers a callback to be invoked when the connection
>>   * is closed. This callback is invoked when there is any
>> @@ -1428,9 +1428,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);
>> +    }
>
> Looks good but I think we should improve the documentation of this API
> to explicitly state that the @freecb is called only on success and if
> the API fails the caller is responsible to free the opaque data.

Will change it in v4.

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

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