[libvirt] [PATCH 7/8] Hide use of timers for domain event dispatch

[Daniel P. Berrange]

On Wed, Dec 14, 2011 at 01:56:36PM -0700, Eric Blake wrote:
> On 12/13/2011 05:38 PM, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > Currently all drivers using domain events need to provide a callback
> > for handling a timer to dispatch events in a clean stack. There is
> > no technical reason for dispatch to go via driver specific code. It
> > could trivially be dispatched directly from the domain event code,
> > thus removing tedious boilerplate code from all drivers
> 
> I think the couple hunks I mentioned in 4/8 remote_driver.c belong in here.
> 
> > 
> > * src/conf/domain_event.c, src/conf/domain_event.h: Internalize
> >   dispatch of events from timer callback
> > * src/libxl/libxl_driver.c, src/lxc/lxc_driver.c,
> >   src/qemu/qemu_domain.c, src/qemu/qemu_driver.c,
> >   src/remote/remote_driver.c, src/test/test_driver.c,
> >   src/uml/uml_driver.c, src/vbox/vbox_tmpl.c,
> >   src/xen/xen_driver.c: Remove all timer dispatch functions
> > ---
> >  src/conf/domain_event.c    |   86 +++++++++++++++++++++++++++++---------------
> >  src/conf/domain_event.h    |   32 +----------------
> >  src/libxl/libxl_driver.c   |   30 +---------------
> >  src/lxc/lxc_driver.c       |   33 +----------------
> >  src/qemu/qemu_domain.c     |   25 -------------
> >  src/qemu/qemu_driver.c     |    5 +--
> >  src/remote/remote_driver.c |   37 +------------------
> >  src/test/test_driver.c     |   32 +----------------
> >  src/uml/uml_driver.c       |   33 +----------------
> >  src/vbox/vbox_tmpl.c       |   45 +----------------------
> >  src/xen/xen_driver.c       |   40 +--------------------
> >  11 files changed, 66 insertions(+), 332 deletions(-)
> 
> More impressive deletion numbers.
> 
> > @@ -1230,10 +1244,11 @@ void virDomainEventDispatch(virDomainEventPtr event,
> >  }
> >  
> >  
> > -void virDomainEventQueueDispatch(virDomainEventQueuePtr queue,
> > -                                 virDomainEventCallbackListPtr callbacks,
> > -                                 virDomainEventDispatchFunc dispatch,
> > -                                 void *opaque)
> > +static void
> > +virDomainEventQueueDispatch(virDomainEventQueuePtr queue,
> 
> In most cases, you changed things to start the function name in column 1
> (with the return type in previous line) - I actually like this style.
> 
> > @@ -1266,10 +1281,23 @@ virDomainEventStateQueue(virDomainEventStatePtr state,
> >      virDomainEventStateUnlock(state);
> >  }
> >  
> > -void
> > -virDomainEventStateFlush(virDomainEventStatePtr state,
> > -                         virDomainEventDispatchFunc dispatchFunc,
> > -                         void *opaque)
> > +
> > +static void virDomainEventStateDispatchFunc(virConnectPtr conn,
> 
> ...but here's a case where the diff makes it look at first glance like
> you converted in the opposite direction.  A closer look shows that you
> kept the layout of virDomainEventStateFlush intact, but added a new
> function, with the new function not using consistent layout, but it
> would still be worth consistent layout here.
> 
> > @@ -952,11 +928,7 @@ libxlStartup(int privileged) {
> >      }
> >      VIR_FREE(log_file);
> >  
> > -    libxl_driver->domainEventState = virDomainEventStateNew(
> > -                                                        libxlDomainEventFlush,
> > -                                                        libxl_driver,
> > -                                                        NULL,
> > -                                                        false);
> > +    libxl_driver->domainEventState = virDomainEventStateNew(true);
> 
> Why the change from false to true?
> 
> > @@ -326,10 +325,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags)
> >          return VIR_DRV_OPEN_ERROR;
> >      }
> >  
> > -    if (!(priv->domainEvents = virDomainEventStateNew(xenDomainEventFlush,
> > -                                                      priv,
> > -                                                      NULL,
> > -                                                      NULL))) {
> > +    if (!(priv->domainEvents = virDomainEventStateNew(true))) {
> 
> Why the change from false (once you fix my comment in 1/8) to true?

Both these drivers run inside the daemon where the event loop is always
expected to be present, so using 'false' was wrong.


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