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

[John Ferlan]

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

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.

John

>  
>      return 0;
>  
>