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@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@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);
+    }
 
     return 0;
 
-- 
2.13.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy@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;
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>> Reviewed-by: Boris Fiuczynski <fiuczy@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.

But I can understand, there is surely someone who might think "hey, the
loop is not running yet, the call was successful => I can still use the
data".

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

Yes, definitely. But a memory leak is still a memory leak… So I'd like
it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an
hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we
would change this (as I suggested in v1), at least our remote_driver
would be fenced against a memory leak (even without this patch). But
this would be an API change :(

doRemoteOpen:
    …
    priv->serverCloseCallback =  remoteConnectSupportsFeatureUnlocked(conn,
                                    priv,
                                    VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
    if (!priv->serverCloseCallback) {
        VIR_INFO("Close callback registering isn't supported "
        "by the remote side.");
    }
    …

Thanks for taking a look!

>
> John
>
>>
>>      return 0;
>>
>>
>
--
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>> Reviewed-by: Boris Fiuczynski <fiuczy@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.

I'm still not sure I understand why the API cannot return a failure, but
Daniel says it cannot.

I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
ping him in the morning...

John

> But I can understand, there is surely someone who might think "hey, the
> loop is not running yet, the call was successful => I can still use the
> data".
> 
>>
>> 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.
> 
> Yes, definitely. But a memory leak is still a memory leak… So I'd like
> it fixed :) Especially, as 'remoteDispatchConnectSupportsFeature' has an
> hard coded “VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK” is supported. If we
> would change this (as I suggested in v1), at least our remote_driver
> would be fenced against a memory leak (even without this patch). But
> this would be an API change :(
> 
> doRemoteOpen:
>     …
>     priv->serverCloseCallback =  remoteConnectSupportsFeatureUnlocked(conn,
>                                     priv,
>                                     VIR_DRV_FEATURE_REMOTE_CLOSE_CALLBACK);
>     if (!priv->serverCloseCallback) {
>         VIR_INFO("Close callback registering isn't supported "
>         "by the remote side.");
>     }
>     …
> 
> Thanks for taking a look!
> 
>>
>> John
>>
>>>
>>>      return 0;
>>>
>>>
>>
> --
> 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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Apr 27, 2018 at 02:16 AM +0200, John Ferlan <jferlan@redhat.com> wrote:
> On 04/26/2018 12:09 PM, Marc Hartmayer wrote:
>> On Thu, Apr 26, 2018 at 05:06 PM +0200, John Ferlan <jferlan@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@linux.vnet.ibm.com>
>>>> Reviewed-by: Boris Fiuczynski <fiuczy@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.
>
> I'm still not sure I understand why the API cannot return a failure, but
> Daniel says it cannot.
>
> I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
> ping him in the morning...
>
> John

Polite ping.

[…snip]

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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>>
>> 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.
>>
>> I'm still not sure I understand why the API cannot return a failure, but
>> Daniel says it cannot.
>>
>> I'm really not sure what to do - maybe Daniel will pipe it - I'll try to
>> ping him in the morning...
>>
>> John
> 
> Polite ping.
> 

Sorry - I totally forgot about that... I did ping him today and
hopefully with this bump we can come to a resolution.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@linux.vnet.ibm.com>
> >>> Reviewed-by: Boris Fiuczynski <fiuczy@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.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé" <berrange@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@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@linux.vnet.ibm.com>
>> >>> Reviewed-by: Boris Fiuczynski <fiuczy@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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Jun 13, 2018 at 10:22 AM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
>> On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:

[…snip…]

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

Polite ping.

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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Aug 07, 2018 at 06:40:46PM +0200, Marc Hartmayer wrote:
> On Wed, Jun 13, 2018 at 10:22 AM +0200, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> > On Mon, Jun 04, 2018 at 06:25 PM +0200, "Daniel P. Berrangé" <berrange@redhat.com> wrote:
> >> On Thu, Apr 26, 2018 at 08:16:54PM -0400, John Ferlan wrote:
> 
> […snip…]
> 
> >>
> >> 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?
> 
> Polite ping.

It is mostly  a revert i think


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list