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

[Ben Guthro]

Daniel Veillard wrote on 10/22/2008 05:14 AM:
> thanks for the changes! I think the prototypes are good to go, maybe a couple
> tiny changes but I can make them myself based on the 'make rebuild'
> output in the doc directory
> functions in src/libvirt.c still need arguments checkings however, but
> I can add that later.

Are these changes worth a 4th round of submissions - or is this something that can be taken care of in a follow-up patch?


>> +/**
>> + * virEventHandleCallback: callback for receiving file handle events
> 
>   description just need to be moved after the parameters, can be
>   trivially done later.
> 
>> + * @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);
>> +/**
>> + * virConnectDomainEventRegister:
>> + * @conn: pointer to the connection
>> + * @cb: callback to the function handling domain events
>> + * @opaque: opaque data to pass on to the callback
>> + *
>> + * Adds a Domain Event Callback
>> + *
>> + * Returns 0 on success, -1 on failure
>> + */
>> +int
>> +virConnectDomainEventRegister(virConnectPtr conn,
>> +                              virConnectDomainEventCallback cb,
>> +                              void *opaque)
>> +{
> 
>    conn still need to be checked  with the standard block
> 
>     if (!VIR_IS_CONNECT(conn)) {
>         virLibConnError(NULL, VIR_ERR_INVALID_CONN, __FUNCTION__);
>         return (-1);
>     }
> 
> like every other public functions with a conn parameter in src/libvirt.c
> What if cb is NULL, that probably need to be rejected
> 
>> +    /* Registering for a domain callback will enable delivery by default */
>> +    if (conn->driver && conn->driver->domainEventRegister)
>> +        return conn->driver->domainEventRegister (conn, cb, opaque);
>> +    return -1;
>> +}
>> +
>> +/**
>> + * 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)
>> +{
> 
>   same thing for VIR_IS_CONNECT and cb being NULL tests
> 
> otherwise looks fine to me.
> 
> Daniel
> 
>