As descriped at https://www.spinics.net/linux/fedora/libvir/msg148562.html,
even when virConnectDomainEventRegisterAny returns -1 there is still a
scenario in which it will trigger the free callback. We fix the problem in the
following way:
(1) implement a function virObjectEventStateSetFreeCB to add freecallback.
(2) call virObjectEventStateSetFreeCB only if the event is successfully added.

Signed-off-by: fangying <fangying1@huawei.com>
---
 src/conf/object_event.c    | 34 ++++++++++++++++++++++++++++++++++
 src/conf/object_event.h    |  7 +++++++
 src/libvirt_private.syms   |  2 +-
 src/remote/remote_driver.c |  4 ++--
 4 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/src/conf/object_event.c b/src/conf/object_event.c
index e5f942f..a770978 100644
--- a/src/conf/object_event.c
+++ b/src/conf/object_event.c
@@ -923,6 +923,40 @@ virObjectEventStateRegisterID(virConnectPtr conn,
 
 
 /**
+ * virObjectEventStateSetFreeCB:
+ * @conn: connection to associate with callback
+ * @state: object event state
+ * @callbackID: ID of the function to remove from event
+ * @freecb: freecb to be added
+ *
+ * Add the callbalck function @freecb for @callbackID with connection @conn,
+ * from @state, for events.
+ */
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+                             virObjectEventStatePtr state,
+                             int callbackID,
+                             virFreeCallback freecb)
+{
+    size_t i;
+    virObjectEventCallbackListPtr cbList;
+
+    virObjectEventStateLock(state);
+    cbList = state->callbacks;
+    for (i = 0; i < cbList->count; i++) {
+        virObjectEventCallbackPtr cb = cbList->callbacks[i];
+
+        if (cb->callbackID == callbackID && cb->conn == conn) {
+            cb->freecb = freecb;
+            break;
+        }
+    }
+
+    virObjectEventStateUnlock(state);
+}
+
+
+/**
  * virObjectEventStateDeregisterID:
  * @conn: connection to associate with callback
  * @state: object event state
diff --git a/src/conf/object_event.h b/src/conf/object_event.h
index 7a9995e..a7d29a0 100644
--- a/src/conf/object_event.h
+++ b/src/conf/object_event.h
@@ -90,4 +90,11 @@ virObjectEventStateSetRemote(virConnectPtr conn,
                              int remoteID)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
 
+void
+virObjectEventStateSetFreeCB(virConnectPtr conn,
+                             virObjectEventStatePtr state,
+                             int callbackID,
+                             virFreeCallback freecb)
+    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
+
 #endif
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 429b095..e9d9cb6 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -791,7 +791,7 @@ virObjectEventStateDeregisterID;
 virObjectEventStateEventID;
 virObjectEventStateNew;
 virObjectEventStateQueue;
-
+virObjectEventStateSetFreeCB;
 
 # conf/secret_conf.h
 virSecretDefFormat;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index d27e96f..71177bd 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -5947,7 +5947,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
 
     if ((count = virDomainEventStateRegisterClient(conn, priv->eventState,
                                                    dom, eventID, callback,
-                                                   opaque, freecb, false,
+                                                   opaque, NULL, false,
                                                    &callbackID,
                                                    priv->serverEventFilter)) < 0)
         goto done;
@@ -5993,7 +5993,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
     }
 
     rv = callbackID;
-
+    virObjectEventStateSetFreeCB(conn, priv->eventState, callbackID, freecb);
  done:
     remoteDriverUnlock(priv);
     return rv;
-- 
2.10.0.windows.1


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

On 06/01/2017 08:25 AM, fangying wrote:
> As descriped at https://www.spinics.net/linux/fedora/libvir/msg148562.html,
> even when virConnectDomainEventRegisterAny returns -1 there is still a
> scenario in which it will trigger the free callback. We fix the problem in the
> following way:
> (1) implement a function virObjectEventStateSetFreeCB to add freecallback.
> (2) call virObjectEventStateSetFreeCB only if the event is successfully added.
> 

Rather than putting a link to some page that may not exist some time in
the future, please describe the issue in the patch. Like you did in your
first version:

https://www.redhat.com/archives/libvir-list/2017-May/msg01089.html

> Signed-off-by: fangying <fangying1@huawei.com>

If you look at other commits in libvirt you'll typically see a "first"
and "last" name... IOW: We prefer a more legal name as opposed to your
current login/email short name.

> ---

If you need to ever place a reference to some web page, when using git
send-email and you can "edit" your patch before sending, you can add
"reviewer comments" right after the "---". That helps see the history of
the patch.  If you have a multiple patch series, that type of
information can go in the cover letter.

>  src/conf/object_event.c    | 34 ++++++++++++++++++++++++++++++++++
>  src/conf/object_event.h    |  7 +++++++
>  src/libvirt_private.syms   |  2 +-
>  src/remote/remote_driver.c |  4 ++--
>  4 files changed, 44 insertions(+), 3 deletions(-)
> 

So this patch doesn't compile using top of tree, please see and follow
the guidelines at:

http://libvirt.org/hacking.html


> diff --git a/src/conf/object_event.c b/src/conf/object_event.c
> index e5f942f..a770978 100644
> --- a/src/conf/object_event.c
> +++ b/src/conf/object_event.c
> @@ -923,6 +923,40 @@ virObjectEventStateRegisterID(virConnectPtr conn,
>  
>  
>  /**
> + * virObjectEventStateSetFreeCB:
> + * @conn: connection to associate with callback
> + * @state: object event state
> + * @callbackID: ID of the function to remove from event
> + * @freecb: freecb to be added
> + *
> + * Add the callbalck function @freecb for @callbackID with connection @conn,
> + * from @state, for events.
> + */
> +void
> +virObjectEventStateSetFreeCB(virConnectPtr conn,
> +                             virObjectEventStatePtr state,
> +                             int callbackID,
> +                             virFreeCallback freecb)
> +{
> +    size_t i;
> +    virObjectEventCallbackListPtr cbList;
> +
> +    virObjectEventStateLock(state);

In particular, this *Lock function no longer exists. It was removed in
libvirt 2.4 by commit id '1827f2ac5d', you should have used
'virObjectLock(state);' instead.

> +    cbList = state->callbacks;
> +    for (i = 0; i < cbList->count; i++) {
> +        virObjectEventCallbackPtr cb = cbList->callbacks[i];
> +
> +        if (cb->callbackID == callbackID && cb->conn == conn) {
> +            cb->freecb = freecb;
> +            break;
> +        }
> +    }
> +
> +    virObjectEventStateUnlock(state);

Similarly virObjectUnlock(state);

> +}
> +
> +
> +/**
>   * virObjectEventStateDeregisterID:
>   * @conn: connection to associate with callback
>   * @state: object event state
> diff --git a/src/conf/object_event.h b/src/conf/object_event.h
> index 7a9995e..a7d29a0 100644
> --- a/src/conf/object_event.h
> +++ b/src/conf/object_event.h
> @@ -90,4 +90,11 @@ virObjectEventStateSetRemote(virConnectPtr conn,
>                               int remoteID)
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> +void
> +virObjectEventStateSetFreeCB(virConnectPtr conn,
> +                             virObjectEventStatePtr state,
> +                             int callbackID,
> +                             virFreeCallback freecb)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4);
> +
>  #endif
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 429b095..e9d9cb6 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -791,7 +791,7 @@ virObjectEventStateDeregisterID;
>  virObjectEventStateEventID;
>  virObjectEventStateNew;
>  virObjectEventStateQueue;
> -

Spurious deletion. Follow the model in the module.  Each .h file has a
sorted list of function names followed by 2 blank lines followed by the
next .h file.

> +virObjectEventStateSetFreeCB;
>  
>  # conf/secret_conf.h
>  virSecretDefFormat;
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index d27e96f..71177bd 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -5947,7 +5947,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
>  
>      if ((count = virDomainEventStateRegisterClient(conn, priv->eventState,
>                                                     dom, eventID, callback,
> -                                                   opaque, freecb, false,
> +                                                   opaque, NULL, false,
>                                                     &callbackID,
>                                                     priv->serverEventFilter)) < 0)
>          goto done;
> @@ -5993,7 +5993,7 @@ remoteConnectDomainEventRegisterAny(virConnectPtr conn,
>      }
>  
>      rv = callbackID;
> -
> +    virObjectEventStateSetFreeCB(conn, priv->eventState, callbackID, freecb);

After reading Daniel's response to your initial posting and doing just a
small amount of investigating - it would seem that perhaps this may fix
one instance, but there are multiple callers pass a @freecb to a
vir*EventStateRegisterClient API, make a remote "call" that could fail,
and then call virObjectEventStateDeregisterID which would conceivably
have the same problem.

So instead of adjusting each of those to have this type set the freecb,
I think altering virObjectEventStateDeregisterID to take a boolean that
would inhibit calling the cb->freecb function may be a better approach.
For callers to virObjectEventStateDeregisterID from some
remoteConnect*EventDeregister* function, the boolean would be false.

IOW: Change virObjectEventStateDeregisterID to add a 4th parameter "bool
inhibitFreeCb". It's "true" on *error* paths from the remote calls
because returning -1 to the caller indicates that the caller needs to
free their opaque data since the freecb wouldn't be run, while calls
from various removeConnect*EventDeregister* APIs the parameter would be
"false" and the logic prior to calling the cb->freecb function is
altered to be "if (!inhibitFreeCb && cb->freecb)", with perhaps a
comment before it indicating that inhibitFreeCb would be used on error
paths to ensure/avoid the double free problem.

HTH,

John



>   done:
>      remoteDriverUnlock(priv);
>      return rv;
> 

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