[libvirt] Re: [PATCH 01/12] Domain Events - Public API
Daniel Veillard
veillard at redhat.com
Tue Oct 21 15:04:47 UTC 2008
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
Basically looks fine with however a few things in-line
> +
> +typedef int (*virConnectDomainEventCallback)(virConnectPtr conn,
> + virDomainPtr dom,
> + int event,
> + void *opaque);
> +
> +int virConnectDomainEventRegister(virConnectPtr conn,
> + virConnectDomainEventCallback cb,
> + void *opaque);
> +
> +int virConnectDomainEventDeregister(virConnectPtr conn,
> + virConnectDomainEventCallback cb);
Please put comments for all function type definitions
this is needed for generating the docs, bindings, etc...
The comment for the functions themselves should be set in the C file.
Basically the comment for a construct should be at the place where the
construct is defined.
> +/**
> + * 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);
Also give name to all parameters, even if C allows passing only the
type, we really need names and comments for all of them
> +
> +typedef int (*virEventAddTimeoutFunc)(int, virEventTimeoutCallback, void *);
> +typedef void (*virEventUpdateTimeoutFunc)(int, int);
> +typedef int (*virEventRemoveTimeoutFunc)(int);
Same thing
And similar for libvirt.h.in
> diff --git a/src/libvirt.c b/src/libvirt.c
[...]
> +/**
> + * virConnectDomainEventDeregister:
> + * @conn: pointer to the connection
> + * @cb: callback to the function handling domain events
> + *
> + * Removes a Domain Event Callback
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int virConnectDomainEventDeregister(virConnectPtr conn, virConnectDomainEventCallback cb)
> +{
the following comment is part of the function description, move it
in the function comment block, and please keep lines less than 80
columns.
Also stylistic we use
int
virFunction(...)
{
for indenting and this kind of things the HACKING file has an indent
description which can be used to automate most of this :-)
> + /* De-registering for a domain callback will disable delivery of this event type*/
For any part of the public API, all parameters must be checked as
thorougthly as possible, there is macros to test active connections
reuse them. Maybe cb should be checked for NULL, if it is allowed
then the function description should indicate the semantic. of passing
NULL.
> + if (conn->driver && conn->driver->domainEventDeregister)
> + return conn->driver->domainEventDeregister (conn, cb);
> +
> + return -1;
> +}
> +
> +/**
> + * virDispatchDomainEvent:
> + * @dom: the domain
> + * @event: the domain event code
> + *
> + * Internal function by which drivers to dispatch domain events.
> + */
> +void virDispatchDomainEvent(virDomainPtr dom,
> + int event)
> +{
parameter check
> + if (dom->conn && dom->conn->driver &&
> + dom->conn->driver->domainEventDispatch)
> + dom->conn->driver->domainEventDispatch(dom, event);
> +}
> +
> +/**
> + * virDomainEventCallbackListRemove:
missing @conn ... not important as it's private but for completion
> + * @cbList: the list
> + * @callback: the callback to remove
> + *
[...]
> +/**
> + * virDomainEventCallbackListAdd:
same thing
> + * @cbList: the list
> + * @callback: the callback to add
> + * @opaque: opaque data tio pass to callback
> + *
> + * Internal function to add a callback from a virDomainEventCallbackListPtr
> + */
> +int __virDomainEventCallbackListAdd(virConnectPtr conn,
> + virDomainEventCallbackListPtr cbList,
> + virConnectDomainEventCallback callback,
> + void *opaque)
> +{
> + virDomainEventCallbackPtr event;
> + int n;
> +
> + /* Check incoming */
> + if ( !cbList ) {
> + return -1;
> + }
> +
> + /* check if we already have this callback on our list */
> + for( n=0; n < cbList->count; n++ )
> + {
> + if(cbList->callbacks[n]->cb == callback &&
> + conn == cbList->callbacks[n]->conn) {
> + DEBUG0("WARNING: Callback already tracked");
> + return -1;
> + }
> + }
again, purely stylistic but libvirt code uses
for (n=0; n < cbList->count; n++) {
if ((cbList->callbacks[n]->cb == callback) &&
(conn == cbList->callbacks[n]->conn)) {
i.e. different spacing, fully parenthesize testing expressings instead
of relying on operator priorities
> + /* Allocate new event */
> + if (VIR_ALLOC(event) < 0) {
> + DEBUG0("Error allocating event");
> + return -1;
> + }
> + event->conn = conn;
> + event->cb = callback;
> + event->opaque = opaque;
> +
> + /* Make space on list */
> + n = cbList->count;
> + if (VIR_REALLOC_N(cbList->callbacks,
> + n + 1) < 0) {
test fits in one line :-)
> + DEBUG0("Error reallocating list");
> + VIR_FREE(event);
> + return -1;
> + }
> +
> + event->conn->refs++;
> +
> + cbList->callbacks[n] = event;
> + cbList->count++;
> + return 0;
> +}
> +
> +/**
> + * virDomainEventQueueFree:
> + * @queue: fron of queue
> + *
> + * Free the memory in the queue. We process this like a list here
> + */
> +void virDomainEventQueueFree(virDomainEventQueuePtr queue)
> +{
> + int i;
need to check all parameters. For structure passed down to the API as
pointers, please add a magic field, a definition for the magic value and
a macro VIR_IS_DOMAIN_EVENT_QUEUE allowing to trivially test the input
parameter.
> +/**
> + * virDomainEventCallbackQueuePop:
> + * @evtQueue: the queue of events
> + *
> + * Internal function to pop off, and return the front of the queue
if it's really internal it should be made static, or start with __
> + * NOTE: The caller is responsible for freeing the returned object
> + *
> + * Returns: virDomainEventPtr on success NULL on failure.
> + */
> +virDomainEventPtr virDomainEventCallbackQueuePop(virDomainEventQueuePtr evtQueue)
> +{
> + virDomainEventPtr ret;
> +
> + if(!evtQueue || evtQueue->count == 0 )
> + return NULL;
> +
> + ret = evtQueue->events[0];
> +
> + memmove(evtQueue->events,
> + evtQueue->events + 1,
> + sizeof(*(evtQueue->events)) *
> + (evtQueue->count - 1));
> +
> + if (VIR_REALLOC_N(evtQueue->events,
> + evtQueue->count - 1) < 0) {
> + ; /* Failure to reduce memory allocation isn't fatal */
> + }
While this is correct code, I start to wonder if the event queue
shoudn't avoid realloc'ing twice per event, that's probably one of the
case where a size of the event queue allocated should be kept.
> + evtQueue->count--;
> +
> + return ret;
> +}
> +
> +/**
> + * virDomainEventCallbackQueuePush:
> + * @evtQueue: the dom event queue
> + * @dom: the domain to add
> + * @event: the event to add
> + *
> + * Internal function to push onto the back of an virDomainEventQueue
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +int virDomainEventCallbackQueuePush(virDomainEventQueuePtr evtQueue,
> + virDomainPtr dom,
> + virDomainEventType event,
> + virConnectDomainEventCallback cb,
> + void *opaque)
> +{
> + virDomainEventPtr domEvent;
> +
> + /* Check incoming */
> + if ( !evtQueue ) {
> + return -1;
Ah :-) , good idea, but implement the magic + macro thing for testing
:-)
> + }
> +
> + /* Allocate new event */
> + if (VIR_ALLOC(domEvent) < 0) {
> + DEBUG0("Error allocating event");
> + return -1;
> + }
> + domEvent->dom = dom;
> + domEvent->event = event;
> + domEvent->cb = cb;
> + domEvent->opaque = opaque;
> +
> + /* Make space on queue */
> + if (VIR_REALLOC_N(evtQueue->events,
> + evtQueue->count + 1) < 0) {
> + DEBUG0("Error reallocating queue");
> + VIR_FREE(domEvent);
> + return -1;
> + }
Do we really want to allow the event queue to grow indefinitely
if the events are not read. I guess something is needed here, or at
least a comment with a TODO
> + evtQueue->events[evtQueue->count] = domEvent;
> + evtQueue->count++;
> + return 0;
> +}
> diff --git a/src/libvirt_sym.version b/src/libvirt_sym.version
> index 3cc4505..5776cf9 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,8 @@
> __virReallocN;
> __virFree;
>
> + __virDomainEventCallbackListFree;
> + __virDomainEventCallbackListAdd;
> + __virDomainEventCallbackListRemove;
> local: *;
> };
Overall the principle of the APIs sounds fine but there is some
cleaning needed before really exposing them.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel at veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
More information about the libvir-list
mailing list