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

[Daniel P. Berrange]

On Tue, Oct 21, 2008 at 03:11:08PM -0400, Ben Guthro wrote:
> [PATCH 01/12] Domain Events - Public API
> 
> 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

> +
> +/**
> + * virEventHandleType:
> + *
> + * a virEventHandleType is used similar to POLLxxx FD events, but is specific
> + * to libvirt. A client app must translate to, and from POLL events when using
> + * this construct.
> + */
> +typedef enum {
> +    VIR_EVENT_HANDLE_READABLE  = (1 << 0),
> +    VIR_EVENT_HANDLE_WRITABLE  = (1 << 1),
> +    VIR_EVENT_HANDLE_ERROR     = (1 << 2),
> +    VIR_EVENT_HANDLE_HANGUP    = (1 << 3),
> +} virEventHandleType;
> +
> +/**
> + * virEventHandleCallback: callback for receiving file handle events
> + *
> + * @fd: file handle on which the event occurred
> + * @events: bitset of events from virEventHandleType constants
> + * @opaque: user data registered with handle
> + */
> +typedef void (*virEventHandleCallback)(int fd, virEventHandleType events,
> +                                       void *opaque);

We avoid using explicit enum types in any public API because they are
not ABI safe in terms of their size. In any case, the 'events' parameter
is atually a bit-mask of zero or more of the virEventHandleType constants,
so the use of the enum isn't technically correct.

> +
> +/**
> + * virEventAddHandleFunc:
> + * @fd: file descriptor to listen on
> + * @event: events on which to fire the callback
> + * @cb: the callback to be called
> + * @opaque: user data to pass to the callback
> + *
> + * Part of the EventImpl, this callback Adds a file handle callback to
> + *  listen for specific events
> + */
> +typedef int (*virEventAddHandleFunc)(int fd, virEventHandleType event,
> +                                     virEventHandleCallback cb, void *opaque);

Here too we should just use  'int events' as a bitmask

> +
> +/**
> + * virEventUpdateHandleFunc:
> + * @fd: file descriptor to modify
> + * @event: new events to listen on
> + *
> + * Part of the EventImpl, this user-provided callback is notified when
> + * events to listen on change
> + */
> +typedef void (*virEventUpdateHandleFunc)(int fd, virEventHandleType event);

Same 'int events' here too.

> +
> +
> +int
> +__virEventHandleTypeToPollEvent(virEventHandleType events)
> +{
> +    int ret = 0;
> +    if(events & VIR_EVENT_HANDLE_READABLE)
> +        ret |= POLLIN;
> +    if(events & VIR_EVENT_HANDLE_WRITABLE)
> +        ret |= POLLOUT;
> +    if(events & VIR_EVENT_HANDLE_ERROR)
> +        ret |= POLLERR;
> +    if(events & VIR_EVENT_HANDLE_HANGUP)
> +        ret |= POLLHUP;
> +    return ret;
> +}
> +
> +virEventHandleType
> +__virPollEventToEventHandleType(int events)
> +{
> +    virEventHandleType ret = 0;
> +    if(events & POLLIN)
> +        ret |= VIR_EVENT_HANDLE_READABLE;
> +    if(events & POLLOUT)
> +        ret |= VIR_EVENT_HANDLE_WRITABLE;
> +    if(events & POLLERR)
> +        ret |= VIR_EVENT_HANDLE_ERROR;
> +    if(events & POLLHUP)
> +        ret |= VIR_EVENT_HANDLE_HANGUP;
> +    return ret;
> +}

Since these two functions are only used by the libvirtd daemon at
this point, I think its better to just put them in qemud/event.c.
This will avoid need to add conditionals here to disable their
build on Mingw/Windows.

> diff --git a/src/libvirt_sym.version b/src/libvirt_sym.version
> index 3cc4505..0297c9c 100644
> --- a/src/libvirt_sym.version
> +++ b/src/libvirt_sym.version
> @@ -147,6 +147,10 @@
>  	virStorageVolGetXMLDesc;
>  	virStorageVolGetPath;
>  
> +        virEventRegisterImpl;
> +        virConnectDomainEventRegister;
> +        virConnectDomainEventDeregister;
> +
>          /* Symbols with __ are private only
>             for use by the libvirtd daemon.
>             They are not part of stable ABI
> @@ -167,8 +171,6 @@
>  	__virGetStoragePool;
>  	__virGetStorageVol;
>  
> -	__virEventRegisterImpl;
> -
>  	__virStateInitialize;
>  	__virStateCleanup;
>  	__virStateReload;
> @@ -198,5 +200,7 @@
>          __virReallocN;
>          __virFree;
>  
> +        __virEventHandleTypeToPollEvent;
> +        __virPollEventToEventHandleType;

These will also be unnecccessary if we put them in qemud/event.c

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