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

[Cole Robinson]

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

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.

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 :/

> 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? This
comment makes it sound like freecb() is invoked immediately when
virConnectRegisterCloseCallback returns 0

Hopefully I'm not confusing things more :)

- 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;
>  
>