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;