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@xxxxxxxxxx>
> >
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list