[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/3] Expose event loop implementation as a public API



On Thu, Mar 03, 2011 at 04:13:08PM -0700, Eric Blake wrote:
> On 03/03/2011 07:21 AM, Daniel P. Berrange wrote:
> > Not all applications have an existing event loop they need
> > to integrate with. Forcing them to implement the libvirt
> > event loop integration APIs is an undue burden. This just
> > exposes our simple poll() based implementation for apps
> > to use. So instead of calling
> > 
> >    virEventRegister(....callbacks...)
> > 
> > The app would call
> > 
> >    virEventRegisterDefaultImpl()
> > 
> > And then have a thread somewhere calling
> > 
> >     static bool quit = false;
> >     ....
> >     while (!quit)
> >       virEventRunDefaultImpl()
> > 
> > * daemon/libvirtd.c, tools/console.c,
> >   tools/virsh.c: Convert to public event loop APIs
> > * include/libvirt/libvirt.h.in, src/libvirt_private.syms: Add
> >   virEventRegisterDefaultImpl and virEventRunDefaultImpl
> > * src/util/event.c: Implement virEventRegisterDefaultImpl
> >   and virEventRunDefaultImpl using poll() event loop
> > * src/util/event_poll.c: Add full error reporting
> > * src/util/virterror.c, include/libvirt/virterror.h: Add
> >   VIR_FROM_EVENTS
> > ---
> >  daemon/libvirtd.c            |   12 +----
> >  include/libvirt/libvirt.h.in |    3 +
> >  include/libvirt/virterror.h  |    1 +
> >  src/libvirt_private.syms     |    8 ----
> >  src/libvirt_public.syms      |    6 +++
> >  src/util/event.c             |   94 +++++++++++++++++++++++++++++++++++++++++-
> >  src/util/event_poll.c        |   24 ++++++++++-
> >  src/util/virterror.c         |    3 +
> >  tools/console.c              |    3 +-
> >  tools/virsh.c                |    9 +---
> >  10 files changed, 133 insertions(+), 30 deletions(-)
> 
> You need to squash this in to keep 'make syntax-check' happy:
> 
> diff --git i/po/POTFILES.in w/po/POTFILES.in
> index 9852f97..1ed2765 100644
> --- i/po/POTFILES.in
> +++ w/po/POTFILES.in
> @@ -88,6 +88,7 @@ src/util/cgroup.c
>  src/util/command.c
>  src/util/conf.c
>  src/util/dnsmasq.c
> +src/util/event_poll.c
>  src/util/hooks.c
>  src/util/hostusb.c
>  src/util/interface.c
> 
> > +++ b/include/libvirt/virterror.h
> > @@ -79,6 +79,7 @@ typedef enum {
> >      VIR_FROM_SYSINFO = 37,	/* Error from sysinfo/SMBIOS */
> >      VIR_FROM_STREAMS = 38,	/* Error from I/O streams */
> >      VIR_FROM_VMWARE = 39,	/* Error from VMware driver */
> > +    VIR_FROM_EVENT = 40,       /* Error from event loop impl */
> >  } virErrorDomain;
> 
> Hmm, the line before had TAB, but your line has spaces, which makes it
> render odd in my reply.  Preferences on which whitespace we should be
> using there?  But any cleanup should be a separate patch.

Odd, I thought our make syntax-check blocked all use of TAB
in our files.

> > +++ b/src/libvirt_public.syms
> > @@ -424,4 +424,10 @@ LIBVIRT_0.8.8 {
> >          virConnectGetSysinfo;
> >  } LIBVIRT_0.8.6;
> >  
> > +LIBVIRT_0.9.0 {
> > +    global:
> > +        virEventRegisterDefaultImpl;
> > +        virEventRunDefaultImpl;
> > +} LIBVIRT_0.8.8;
> 
> So we're all in agreement that there's enough refactoring and other
> goodness going in to call the next version 0.9.0 :)

There's far more to come from me too :-)

> > +int virEventRunDefaultImpl(void)
> > +{
> > +    VIR_DEBUG0("");
> 
> Why ""?  A timestamp in the log without contents looks suspicious;
> should we add some contents, such as "event loop started"?

It isn't just the timestamp. The log will contain the function name,
which is what I really want to see.

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]