[libvirt] Re: [PATCH 01/12] Domain Events - Public API

[Daniel P. Berrange]

On Fri, Oct 17, 2008 at 11:53:53AM -0400, Ben Guthro wrote:
>  This patch does the following:
>    -implements the Event register/deregister code
>    -Adds some callback lists, and queue functions used by drivers
>    -Move EventImpl definitions into the public
> 
>  include/libvirt/libvirt.h    |   59 +++++++++
>  include/libvirt/libvirt.h.in |   59 +++++++++
>  src/libvirt.c                |  256 +++++++++++++++++++++++++++++++++++++++++++
>  src/libvirt_sym.version      |    9 +
>  4 files changed, 381 insertions(+), 2 deletions(-)


> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 3624367..6af5329 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> +
> +/**
> + * virEventHandleCallback: callback for receiving file handle events
> + *
> + * @fd: file handle on which the event occurred
> + * @events: bitset of events from POLLnnn constants
> + * @opaque: user data registered with handle
> + */
> +typedef void (*virEventHandleCallback)(int fd, int events, void *opaque);
> +
> +typedef int (*virEventAddHandleFunc)(int, int, virEventHandleCallback, void *);
> +typedef void (*virEventUpdateHandleFunc)(int, int);
> +typedef int (*virEventRemoveHandleFunc)(int);

For the handle here, we need to define some constants for the events.
The current internal impl assumed that the events parameter was just
using the POLLnn constants, but not all users of libvirt will neccessarily
use poll() for their event loop. So I think we should define something
like

 enum {
  VIR_EVENT_HANDLE_READABLE  = (1 << 0)
  VIR_EVENT_HANDLE_WRITABLE  = (1 << 1)
  VIR_EVENT_HANDLE_ERROR     = (1 << 2)
  VIR_EVENT_HANDLE_HANGUP    = (1 << 2)
 };

> +
> +/**
> + * virDispatchDomainEvent:
> + * @dom: the domain
> + * @event: the domain event code
> + *
> + * Internal function by which drivers to dispatch domain events.
> + */
> +void virDispatchDomainEvent(virDomainPtr dom,
> +                            int event)
> +{
> +    if (dom->conn && dom->conn->driver &&
> +        dom->conn->driver->domainEventDispatch)
> +        dom->conn->driver->domainEventDispatch(dom, event);
> +}

I don't think this helper method is needed in libvirt.c - i'll show
why in my reply to the QEMU driver patch.

Other than these two points, the rest of this patch looks sound.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|