Re: [libvirt] [PATCH] events: Don't fail on registering events for two different domains

On 06/27/2012 06:12 AM, Michal Privoznik wrote: > virConnectDomainEventRegisterAny() takes a domain as an argument. > So it should be possible to register the same event (be it > VIR_DOMAIN_EVENT_ID_LIFECYCLE for example) for two different domains. > That is, we need to take domain into account when searching for > duplicate event being already registered. > --- > src/conf/domain_event.c | 6 +++++- > 1 files changed, 5 insertions(+), 1 deletions(-) > > diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c > index 4ecc413..3cfd940 100644 > --- a/src/conf/domain_event.c > +++ b/src/conf/domain_event.c > @@ -363,7 +363,11 @@ virDomainEventCallbackListAddID(virConnectPtr conn, > for (i = 0 ; i < cbList->count ; i++) { > if (cbList->callbacks[i]->cb == VIR_DOMAIN_EVENT_CALLBACK(callback) && > cbList->callbacks[i]->eventID == eventID && > - cbList->callbacks[i]->conn == conn) { > + cbList->callbacks[i]->conn == conn && > + ((dom && cbList->callbacks[i]->dom && > + memcmp(cbList->callbacks[i]->dom->uuid, > + dom->uuid, VIR_UUID_BUFLEN) == 0) || > + (!dom && !cbList->callbacks[i]->dom))) { This misses the case of registering a catchall against NULL domain then attempting to re-register the same event against a specific domain (which one of the two would fire?). It also misses the case of registering a domain-specific handler, then attempting to broaden things into a global handler. I think the idea of double registration makes sense (in particular, if I have a per-domain callback, but then want to switch to global, I would rather have a window with both handlers at once than a window with no handler at all), but I have not tested whether we actually handle it by firing both the global and domain-specific callback. If we are happy with double registration, then your patch looks correct on the registration side (although it may be incomplete, if we mishandle double firing). On the other hand, if we want to prevent double registration, then you need to further forbid registration when ((dom && !cbList->callbacks[i]->dom) || (!dom && cblist->callbacks[i]->dom)).