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))) {

On 27.06.2012 14:36, Eric Blake wrote: > 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. Yes, but that's intentional. In both cases both events are fired. > > 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. I've tested it and it works.

On 27.06.2012 14:46, Daniel P. Berrange wrote: > On Wed, Jun 27, 2012 at 02:44:02PM +0200, Michal Privoznik wrote: >> On 27.06.2012 14:36, Eric Blake wrote: >>> 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. >> >> Yes, but that's intentional. In both cases both events are fired. >> >>> >>> 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. >> >> I've tested it and it works. > > How ? The remote driver does not ever pass the virDomainPtr arg > over the wire, and it restricts itself to 1 single callback per > event type. Only the client side drivers (test, esx, virtualbox) > could possibly work, but not KVM, LXC, Xen, UML > > Daniel > I am attaching the reproducer. My output: zippy@bart ~/work/tmp $ gcc repro.c -o repro -lvirt -ggdb3 && LD_LIBRARY_PATH="/home/mprivozn/work/libvirt/libvirt.git/src/.libs/" ./repro qemu:///system myDomainEventCallback2 EVENT: Domain f16(-1) Defined Updated o:cb1 myDomainEventCallback2 EVENT: Domain f17(-1) Defined Updated o:cb1 myDomainEventCallback2 EVENT: Domain f17(-1) Defined Updated o:cb2 So we can see cb1 and cb2 firing up at once (cb1 is registered against NULL, cb2 against f17 domain).

On 27.06.2012 14:12, 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))) { > eventReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("event callback already tracked")); > return -1; > So after all - ACK or NACK?