[Libvir] PATCH 1/3: Split out simple event loop

[Daniel P. Berrange]

On Mon, Jun 18, 2007 at 05:49:59AM -0400, Daniel Veillard wrote:
> On Sun, Jun 17, 2007 at 10:52:55PM +0100, Daniel P. Berrange wrote:
> > The following patch adds a qemud/event.c & qemud/event.h file providing a
> > general purpose event loop built around poll. Users register file handles
> > and associated callbacks, and / or timers. The qemud.c file is changed to
> > make use of these APIs for dealing with server, client, and VM file handles
> > and/or sockets. This decouples much of the QEMU VM I/O code from the main
> > qemud.c daemon code.
> > 
> >  qemud/event.c   |  311 +++++++++++++++++++++++++++++++++++++++++
> >  qemud/event.h   |   13 +
> >  qemud/Makefile.am |    3 
> >  qemud/qemud.c     |  401 +++++++++++++++++++++++++++++-------------------------
> >  qemud/remote.c    |    5 
> >  5 files changed, 546 insertions(+), 187 deletions(-)
> 
>   As I understand taht patch could be applied alone without disturbing
> operations, right ?

Assuming it is bug free, then yes :-)

> > diff -r 93de958458cb qemud/Makefile.am
> > --- a/qemud/Makefile.am	Fri Jun 15 14:27:39 2007 +0000
> > +++ b/qemud/Makefile.am	Sun Jun 17 16:26:41 2007 -0400
> > @@ -16,7 +16,8 @@ libvirt_qemud_SOURCES = \
> >  		buf.c buf.h \
> >  		protocol.h protocol.c \
> >  		remote_protocol.h remote_protocol.c \
> > -		remote.c
> > +		remote.c \
> > +                event.c event.h
> >  #-D_XOPEN_SOURCE=600 -D_XOPEN_SOURCE_EXTENDED=1 -D_POSIX_C_SOURCE=199506L
> >  libvirt_qemud_CFLAGS = \
> >          -I$(top_srcdir)/include -I$(top_builddir)/include $(LIBXML_CFLAGS) \
> > diff -r 93de958458cb qemud/event.c
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/qemud/event.c	Sun Jun 17 16:26:34 2007 -0400
> > @@ -0,0 +1,311 @@
> 
>   In general a severe lack of comment, I assume it's an early review version.

Yep, needs documentation.

> > +int virEventAddHandle(int fd, int events, virEventHandleCallback cb, void *opaque) {
> > +    struct virEventHandle *tmp;
> > +
> > +    printf("Add handle %d %d %p %p\n", fd, events, cb, opaque);
> > +    tmp = realloc(eventLoop.handles, sizeof(struct virEventHandle) * (eventLoop.nhandles+1));
> > +    if (!tmp) {
> > +        return -1;
> > +    }
> 
>   Hum, realloc( , n+1) always leave me with a unpleasant feeling. I really
> prefer a current usage size, an allocated size and growing by doubling the
> block only when needed.

Based on my debugging this is probably a good idea - with the TLS stuff we
tend to addd & remove handles *alot* because the direction of traffic is
switching between TX & RX all the time.

> > -static int qemudDispatchSignal(struct qemud_server *server)
> > -{
> > +static void qemudDispatchSignalEvent(int fd ATTRIBUTE_UNUSED,
> > +                                     int events ATTRIBUTE_UNUSED,
> > +                                     void *opaque) {
> > +    struct qemud_server *server = (struct qemud_server *)opaque;
> >      unsigned char sigc;
> >      struct qemud_vm *vm;
> >      struct qemud_network *network;
> > @@ -194,7 +204,7 @@ static int qemudDispatchSignal(struct qe
> >      if (read(server->sigread, &sigc, 1) != 1) {
> >          qemudLog(QEMUD_ERR, "Failed to read from signal pipe: %s",
> >                   strerror(errno));
> > -        return -1;
> > +        return;
> >      }
> >  
> >      ret = 0;
> > @@ -266,7 +276,8 @@ static int qemudDispatchSignal(struct qe
> >          break;
> >      }
> >  
> > -    return ret;
> > +    if (ret != 0)
> > +        server->shutdown = 1;
> >  }
> 
> is asynchronous error handling really better there ?

It is unavoidable in this context actually, since this is a callback
invoked from within the event loop, there isn't any meaningful caller
to deal with a return value. The event loop itself checks on the
server->shutdown flag on each iteration, so the end result is the
same as previous code.

> 
> >  static int qemudSetCloseExec(int fd) {
> > @@ -474,19 +485,16 @@ static int qemudListenUnix(struct qemud_
> >      }
> [...]
> > diff -r 93de958458cb qemud/remote.c
> > --- a/qemud/remote.c	Fri Jun 15 14:27:39 2007 +0000
> > +++ b/qemud/remote.c	Sun Jun 17 16:26:54 2007 -0400
> > @@ -691,7 +691,7 @@ remoteDispatchDomainGetInfo (struct qemu
> >          remoteDispatchError (client, req, "domain not found");
> >          return -2;
> >      }
> > -
> > +    printf("------------------------------------- %d %s\n", dom->id, dom->name);
> 
>   I assume those are debugging left over, right ?

Opps, yes, wrong version of the patch.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|