On 01/06/2014 05:27 PM, Eric Blake wrote:
> Right now, the older virConnectDomainEventRegister (takes a
> function pointer, returns 0 on success) and the newer
> virConnectDomainEventRegisterID (takes an eventID, returns a
> callbackID) share the underlying implementation (the older
> API ends up consuming a callbackID for eventID 0 under the
> hood). We implemented that by a lot of copy and pasted
> code between object_event.c and domain_event.c, according to
> whether we are dealing with a function pointer or an eventID.
> However, our copy and paste is not symmetric. Consider this
> sequence:
>
> id1 = virConnectDomainEventRegisterAny(conn, dom,
> VIR_DOMAIN_EVENT_ID_LIFECYCLE,
> VIR_DOMAIN_EVENT_CALLBACK(callback), NULL, NULL);
> virConnectDomainEventRegister(conn, callback, NULL, NULL);
> virConnectDomainEventDeregister(conn, callback);
> virConnectDomainEventDeregsiterAny(conn, id1);
>
> the first three calls would succeed, but the third call ended
> up nuking the id1 callbackID (the per-domain new-style handler),
> then the fourth call failed with an error about an unknown
> callbackID, leaving us with the global handler (old-style) still
> live and receiving events. It required another old-style
> deregister to clean up the mess. Root cause was that
> virDomainEventCallbackList{Remove,MarkDelete} were only
> checking for function pointer match, rather than also checking
> for whether the registration was global.
>
> Rather than playing with the guts of object_event ourselves
> in domain_event, it is nicer to add a mapping function for the
> internal callback id, then share common code for event removal.
> For now, the function-to-id mapping is used only internally;
> I thought about whether a new public API to let a user learn
> the callback would be useful, but decided exposing this to the
> user is probably a disservice, since we already publicly
> document that they should avoid the old style, and since this
> patch already demonstrates that older libvirt versions have
> weird behavior when mixing old and new styles.
>
> * src/conf/object_event.c (virObjectEventCallbackLookup)
> (virObjectEventStateCallbackID): New functions.
> (virObjectEventCallbackLookup): Use helper function.
> * src/conf/object_event_private.h (virObjectEventStateCallbackID):
> Declare new function.
> * src/conf/domain_event.c (virDomainEventStateRegister)
> (virDomainEventStateDeregister): Let common code handle the
> complexity.
> (virDomainEventCallbackListRemove)
> (virDomainEventCallbackListMarkDelete)
> (virDomainEventCallbackListAdd): Drop unused functions.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/conf/domain_event.c | 171 ++++------------------------------------
> src/conf/object_event.c | 96 +++++++++++++++++++---
> src/conf/object_event_private.h | 9 +++
> 3 files changed, 109 insertions(+), 167 deletions(-)
>
ACK the pair (6 & 7)... The intro comment to 6 had me do a double take
though thinking I messed up the pair!
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list