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

[Daniel P. Berrange]

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 :|