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

On 12/13/2011 05:38 PM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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?