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@xxxxxxxxxx> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
