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

[Marc Hartmayer]

On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson <crobinso at redhat.com> wrote:
> On 11/14/19 12:44 PM, 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.
>> 
>
>
> Full context:
> v1 subtrhead with jferlan and danpb:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00906.html
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> v2 subthread with john:
> https://www.redhat.com/archives/libvir-list/2018-April/msg02534.html
>
> My first thought is, why not just make this API start raising an error
> if it isn't supported?
>
> But you tried that here:
> https://www.redhat.com/archives/libvir-list/2018-March/msg00405.html

First, thanks for doing all the “history research”.

>
> I'm not really sure I buy the argument that we can't change the
> semantics of the API here. This is the only callback API that seems to
> not raise an explicit error. It's documented to raise an error. And
> there's possible memory leak due the ambiguity.

If we’re doing this so let’s fix the behavior of
'virConnectUnregisterCloseCallback' as well.

>
> Yeah I see that virsh needs to be updated to match. In practice virsh
> shouldn't be a problem: this issue will only hit for local drivers, and
> virsh and client library will be updated together for that case.
>
> In theory if a python app is using this API and not expecting an
> exception, it could cause a regression. But it's also informing them
> that, hey, that connection callback you requested wasn't working in the
> first place. So it's arguably a correctness issue.
>
> So IMO we should be able to adjust this to return a proper error.
>
>
> BUT, if we stick with this direction, then we need to extend the doc
> text here to describe all of this:
>
> * Returns -1 if the driver can support close callback, but registering
> one failed. User must free opaque?
> * Returns 0 if the driver does not support close callback. We will free
> data for you
> * Returns 0 if the driver successfully registered a close callback. When
> that callback is triggered, opaque will be free'd
>
> But that sounds pretty nutty IMO :/

I know…

>
>> Therefore call directly @freecb if the connectRegisterCloseCallback
>> API is not supported by the driver used by the connection and update
>> the documentation.
>> 
>> Signed-off-by: Marc Hartmayer <mhartmay at linux.ibm.com>
>> ---
>>  src/libvirt-host.c | 16 +++++++++++-----
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/libvirt-host.c b/src/libvirt-host.c
>> index 221a1b7a4353..94383ed449a9 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
>> @@ -1412,7 +1412,9 @@ virConnectIsAlive(virConnectPtr conn)
>>   *
>>   * The @freecb must not invoke any other libvirt public
>>   * APIs, since it is not called from a re-entrant safe
>> - * context.
>> + * context. Only if virConnectRegisterCloseCallback is
>> + * successful, @freecb will be called, otherwise the
>> + * caller is responsible to free @opaque.
>
> This reads wrong to me. If cb() is successfully registered, then
> freecb() is invoked automatically after cb() is called, right?

Yep, or if the callback is unregistered.

> This
> comment makes it sound like freecb() is invoked immediately when
> virConnectRegisterCloseCallback returns 0

I can reword it.

>
> Hopefully I'm not confusing things more :)

No, I appreciate that you’re looking for it.

>
> - Cole
>
>>   *
>>   * Returns 0 on success, -1 on error
>>   */
>> @@ -1428,9 +1430,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);
>> +    }
>>  
>>      return 0;
>>  
>>
>
-- 
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