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

2020-01-21 Thread Daniel P . Berrangé
On Tue, Jan 14, 2020 at 10:34:21AM +0100, Marc Hartmayer wrote:
> On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson  
> wrote:
> > On 12/12/19 8:46 AM, Marc Hartmayer wrote:
> >> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson 
> >>  wrote:
> >>> On 11/14/19 12:44 PM, Marc Hartmayer wrote:
> >
> > danpb has your thinking changed on this? Previous discussion context is
> > in this thread:
> > https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
> >
> > Thanks,
> > Cole
> >
> 
> Polite ping.

Sorry for the ridiculous delay in responding. This has been on my
todo list for ages and I didn't prioritize completing it well enough.
I've sent a mail with my thoughts now.

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



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

2020-01-21 Thread Daniel P . Berrangé
On Wed, Dec 11, 2019 at 08:11:38PM -0500, Cole Robinson wrote:
> 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.

It can raise an error because you are only permitted to register the
close callback once - a duplicated call reports an error. Also any
other invalid parameters result in an error.  So this is not
inconsistent with the idea that registering a close callback is
supported for all drivers.

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

The very fact that we need to update virsh shows that this would
be an API regression. That we only know of virsh having a problem
is not a valid reason. It is only by luck that virt-viewer doesn't
have the same problem, because for inexplicable reasons we didn't
handling the error as an error condition.

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

This is giving apps an uncessary level of impl detail for their
needs.

 * Returns -1: if a close callback was already registered, or
   if one of the parameters was invalid.
 * Returns 0: if the close callback was successfully registered


The driver specific caveat is described earlier in the docs, that
not all drivers will invoke the close callback, as some may not
ever be in a situation where there is a connection is closed. Not
ever invoking the callback is not a bug, as it simply means the
error condition the callback is designed to report has not
arisen.

To fix the leak of te opaque data, we need to partially revert
the change that caused this leak in the first place
88f09b75eb99415c7f2ce3d1b010600ba8e37580.

That commit introduces some per driver handling into the close
callbacks. This is conceptually fine. The mistake was that the

   virConnectCloseCallbackDataPtr closeCallback;

field was moved out of virConnectPtr, and into the per-driver
private structs. This meant that we no longer kept track of
the callback in other drivers, and thus no longer free'd the
opaque data when calling Unregister.

So from the public API entry point I think we need approx
this change:

diff --git a/src/datatypes.h b/src/datatypes.h
index 2d0407f7ec..924ef8d8e9 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -546,6 +546,9 @@ struct _virConnect {
 virError err;   /* the last error */
 virErrorFunc handler;   /* associated handler */
 void *userData; /* the user data */
+
+/* Per-connection close callback */
+virConnectCloseCallbackDataPtr closeCallback;
 };
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virConnect, virObjectUnref);
diff --git a/src/libvirt-host.c b/src/libvirt-host.c
index bc3d1d2803..68517bae9c 100644
--- a/src/libvirt-host.c
+++ b/src/libvirt-host.c
@@ -1444,9 +1444,20 @@ virConnectRegisterCloseCallback(virConnectPtr conn,
 virCheckConnectReturn(conn, -1);
 virCheckNonNullArgGoto(cb, error);
 
+if (virConnectCloseCallbackDataGetCallback(conn->closeCallback) != cb) {
+virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+   _("A different callback was requested"));
+goto error;
+}
+
+virConnectCloseCallbackDataRegister(conn->closeCallback, conn, cb,
+opaque, freecb);
+
 if 

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

2020-01-14 Thread Marc Hartmayer
On Fri, Dec 13, 2019 at 03:32 PM -0500, Cole Robinson  
wrote:
> On 12/12/19 8:46 AM, Marc Hartmayer wrote:
>> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson  
>> wrote:
>>> 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
>> 
>> First, thanks for doing all the “history research”.
>> 
>>>
>>> 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.
>> 
>> If we’re doing this so let’s fix the behavior of
>> 'virConnectUnregisterCloseCallback' as well.
>> 
>>>
>>> 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 :/
>> 
>> I know…
>
> I did a bit more digging. Even the virsh case isn't the biggest deal
> because CloseCallback failing is non-fatal. But like I mentioned before
> it shouldn't realistically be hit in practice because we can expect
> virsh and libvirt-client to be updated in lockstep.
>
> virt-manager, libguestfs, vdsm, kubevirt don't use this API
> nova does (nova/virt/libvirt/host.py) but it has code to catch the error
>
> So IMO this should be changed to have semantics like all the other event
> functions. Yes it's a semantic change, but I see it as explicitly
> erroring in a case that never actually worked. We've made changes like
> that many times, happens with XML validation semi regularly, and the
> UNDEFINE flag changes are other notable examples.
>
> danpb has your thinking changed on this? Previous discussion context is
> in this thread:
> https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html
>
> Thanks,
> Cole
>

Polite ping.

-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
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

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

2019-12-13 Thread Cole Robinson
On 12/12/19 8:46 AM, Marc Hartmayer wrote:
> On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson  
> wrote:
>> 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
> 
> First, thanks for doing all the “history research”.
> 
>>
>> 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.
> 
> If we’re doing this so let’s fix the behavior of
> 'virConnectUnregisterCloseCallback' as well.
> 
>>
>> 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 :/
> 
> I know…

I did a bit more digging. Even the virsh case isn't the biggest deal
because CloseCallback failing is non-fatal. But like I mentioned before
it shouldn't realistically be hit in practice because we can expect
virsh and libvirt-client to be updated in lockstep.

virt-manager, libguestfs, vdsm, kubevirt don't use this API
nova does (nova/virt/libvirt/host.py) but it has code to catch the error

So IMO this should be changed to have semantics like all the other event
functions. Yes it's a semantic change, but I see it as explicitly
erroring in a case that never actually worked. We've made changes like
that many times, happens with XML validation semi regularly, and the
UNDEFINE flag changes are other notable examples.

danpb has your thinking changed on this? Previous discussion context is
in this thread:
https://www.redhat.com/archives/libvir-list/2018-April/msg00116.html

Thanks,
Cole

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

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

2019-12-12 Thread Marc Hartmayer
On Wed, Dec 11, 2019 at 08:11 PM -0500, Cole Robinson  
wrote:
> 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

First, thanks for doing all the “history research”.

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

If we’re doing this so let’s fix the behavior of
'virConnectUnregisterCloseCallback' as well.

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

I know…

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

Yep, or if the callback is unregistered.

> This
> comment makes it sound like freecb() is invoked immediately when
> virConnectRegisterCloseCallback returns 0

I can reword it.

>
> Hopefully I'm not confusing things more :)

No, I appreciate that you’re looking for it.

>
> - 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;
>>  
>>
>
-- 
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: 

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

2019-12-11 Thread 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 
> ---
>  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;
>  
>

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



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

2019-11-14 Thread Marc Hartmayer
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 and update
the documentation.

Signed-off-by: Marc Hartmayer 
---
 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.
  *
  * 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;
 
-- 
2.21.0


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