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

[Pavel Hrdina]

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.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191108/b439da03/attachment-0001.sig>