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

[Marc Hartmayer]

On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé" <berrange at redhat.com> wrote:
> On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
>>
>>
>> On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
>> > On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan at redhat.com> wrote:
>> >> 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.
>> >
>> > I understand your concerns.
>> >
>> >> 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.
>> >
>> > virshReconnect doesn’t pass a @freecb != NULL argument, does it? So the
>> > registered callback has not the responsibility to free the memory.
>> >
>>
>> True virsh uses NULL so it's fine; however, I was thinking about more
>> generically - why would a Register routine with a callback to free
>> memory free the memory upon successful register.
>
> Well the memory passed in 'opaque' is only required to be kept alive for
> as long as the callback is registered, and only documented for use by the
> callback.
>
> If the application wants to access 'opaque' outside the context of the
> callback function, it must take steps to ensure it is still alive in
> whatever thread it using it. This implies the data passed for 'opaque'
> should be ref-counted and they must hold a reference for their own
> usage, separately from the reference assoicated with the callback that
> will be released by @freecb.
>
> That all said, we could take a slightly different approach if we want
> to be paranoid about this
>
> eg move the
>
>     virConnectCloseCallbackDataPtr closeCallback;
>
> out of the driver specific private structs, and put it in the main
> struct _virConnect instead.

This sound like a revert of commit “close callback: move it to driver”
(88f09b75eb99415c). Shall we really do this?

>
> The main libvirt-host.c can add the callback to this itself. THe
> driver code only needs to worry about actually invoking the callback
>
> That would allow us to have freecb() called at the right time for
> all drivers, even if they don't ever use the close callback.
>
>>
>> I'm still not sure I understand why the API cannot return a failure, but
>> Daniel says it cannot.
>
> It can break existing applications using hypervisors that don't
> implement this API, becasue its a change in behaviour. In retrospect
> I wouldn't have written the API in this way today, but we must live
> with the design we have.
>
>
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>
--
Beste Grüße / Kind regards
   Marc Hartmayer

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