[libvirt] [PATCH] events: Don't fail on registering events for two different domains
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; -- 1.7.3.4
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)). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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.
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)).
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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). Michal
On Wed, Jun 27, 2012 at 02:51:43PM +0200, Michal Privoznik wrote:
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).
Ah, I see what's going on. The remote client is only registering a single handler with the server, always with domain==NULL. The filtering of events to individual domains & dispatch of multiple handlers is then done completely client side. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, Jun 27, 2012 at 06:36:14AM -0600, 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.
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)).
You are correct that this patch doesn't actually fix the problem of registering handlers for multiple domains. The core issue is that libvirtd tracks all registered callbacks, so that it can later unregister them. But it only tracks 1 callback per event type. So I don't see how this patch could have actually been tested. Indeed the wire protocol does not even pass the 'virDomainPtr' over the network, so this can't possibly work with a non-NULL domain; Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 06/27/2012 06:45 AM, Daniel P. Berrange wrote:
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)).
You are correct that this patch doesn't actually fix the problem of registering handlers for multiple domains. The core issue is that libvirtd tracks all registered callbacks, so that it can later unregister them. But it only tracks 1 callback per event type. So I don't see how this patch could have actually been tested.
Indeed the wire protocol does not even pass the 'virDomainPtr' over the network, so this can't possibly work with a non-NULL domain;
In the case of qemu, there's two layers of handlers: - libvirt.so registers a local handler (either global or per-domain), and remembers which user callback to fire - the remote hypervisor driver then sends an RPC to register a global handler, but with the callback being a function in the remote driver - when an event is triggered remotely, the global handler registered via RPC sends an RPC event back - libvirt.so then looks at the event (which _does_ have domain information) and decides whether to fire the local callback(s) based on whether the local callback is global or has a per-domain match In other words, I see no reason why the current RPC design shouldn't work; you can still have both global and per-domain callbacks from the point of view of the application, even though the RPC side is using only a single global callback (which is basically installed on a reference-counting principle - if there is one or more local handlers, then the global remote handler exists) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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? Michal
On 06/27/2012 08:22 AM, Michal Privoznik wrote:
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?
ACK - I think we agreed that current behavior of allowing both global and per-domain simultaneous registration is fine, and this preserves that ability. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 27.06.2012 16:26, Eric Blake wrote:
On 06/27/2012 08:22 AM, Michal Privoznik wrote:
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?
ACK - I think we agreed that current behavior of allowing both global and per-domain simultaneous registration is fine, and this preserves that ability.
Okay, pushed then. Thanks! Michal
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik