[libvirt] [PATCH 1/4] add a qemu-specific event register API, to passthough the new events come from qemu

ShaoHe Feng shaohef at linux.vnet.ibm.com
Sun Feb 5 15:42:22 UTC 2012


Thank you, Eric.
I want to improve this feature as follow , in order to simplify the code.

1.  set up a hashtable of event names to a appropriate struct.
       when user register an event,  this event name will be added to 
this hashtable.
       and maybe different connection will add the same event name, so 
the appropriate struct should include all the connections.
2. when qemuMonitorIOProcess receives qemu Monitor event, I want to to 
simplify the  process the qemu-event, not same with the libvirt event.
       if the qeme-event name can be found in the hashtable,  I think I 
should sent the event to the connection client by qemu_protocol.x 
directly, instead of  putting it into the libvirt event queue.
       This is different from  the libvirt events.  The libvirt events 
are putted into the libvirt event queue and then libvirtd will run event 
loop to dispatch these events.

if this is OK, then  I think the hashtable of event names should be a 
global variable.  Is it appropriate that I make the hashtable as a 
member of qemu_driver struct?

I wonder when will  the 0.9.10 be released?


On 02/04/2012 07:29 AM, Eric Blake wrote:
> On 12/16/2011 09:58 AM, shaohef at linux.vnet.ibm.com wrote:
>> From: ShaoHe Feng<shaohef at linux.vnet.ibm.com>
>>
>> Basically, this feature can go along with qemu monitor passthrough.
>> That way, if we use new commands in the monitor that generate new events, we want some way to receive those new events too.
>>
>> Signed-off-by: ShaoHe Feng<shaohef at linux.vnet.ibm.com>
>> ---
>>   include/libvirt/libvirt-qemu.h |   27 ++++
>>   include/libvirt/libvirt.h.in   |    2 +-
>>   src/conf/domain_event.c        |  293 ++++++++++++++++++++++++++++++++++++++--
>>   src/conf/domain_event.h        |   50 ++++++-
>>   src/driver.h                   |   14 ++
>>   src/libvirt-qemu.c             |  189 ++++++++++++++++++++++++++
>>   src/libvirt_private.syms       |    6 +
>>   src/libvirt_qemu.syms          |    5 +
>>   8 files changed, 571 insertions(+), 15 deletions(-)
>>
>> +/**
>> + * virConnectDomainQemuEventCallback:
>> + * @conn: connection object
>> + * @dom: domain on which the event occurred
>> + * @eventName : the name of the unknow or un-implementation event
> s/unknow/unknown/
>
> except that I think this interface should be usable to get the JSON
> argument string of _any_ qemu monitor event, even if libvirt is already
> handling it.  That is, in qemu, I think we want to parse every incoming
> event, and do two things with it:
>
> 1. if the event name has been registered with the qemu-specific event
> registration, send a libvirt-qemu event
> 2. if the event name is known and has a libvirt callback, then do the
> libvirt callback (which might send a libvirt event).
>
> So UNKNOWN is really not the right word to use.  Rather, it is an
> arbitrary qemu event, whether or not libvirt knows about it.
>
>> + * @eventArgs: the content of the unknow or un-implementation event
>> + *
>> + * The callback signature to use when registering for an event of type
>> + * VIR_QEMU_DOMAIN_EVENT_ID_UNKNOWN with virConnectDomainQemuEventRegister()
> This sentence would then be:
>
> The callback signature to use when registering for an arbitrary qemu
> event with virConnectDomainQemuEventRegister().
>
>> + */
>> +typedef void (*virConnectDomainQemuEventCallback)(virConnectPtr conn,
>> +                                                  virDomainPtr dom,
>> +                                                  const char *eventName, /* The JSON event name */
>> +                                                  const char *eventArgs, /* The JSON string of args */
>> +                                                  void *opaque);
> This signature looks reasonable.
>
>> +
>> +int
>> +virConnectDomainQemuEventRegister(virConnectPtr conn,
>> +                                  virDomainPtr dom, /* option to filter */
>> +                                  const char *eventName,  /* JSON event name */
>> +                                  virConnectDomainQemuEventCallback cb,
>> +                                  void *opaque,
>> +                                  virFreeCallback freecb);
> As does this API.
>
>> +int
>> +virConnectDomainQemuEventDeregister(virConnectPtr conn,
>> +                                    int callbackID);
>> +
>>   # ifdef __cplusplus
>>   }
>>   # endif
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 2480add..9fcb400 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -3207,7 +3207,6 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn,
>>                                                         int type,
>>                                                         int status,
>>                                                         void *opaque);
>> -
>>   /**
>>    * virConnectDomainEventDiskChangeReason:
>>    *
>> @@ -3263,6 +3262,7 @@ typedef enum {
>>       VIR_DOMAIN_EVENT_ID_CONTROL_ERROR = 7,   /* virConnectDomainEventGenericCallback */
>>       VIR_DOMAIN_EVENT_ID_BLOCK_JOB = 8,       /* virConnectDomainEventBlockJobCallback */
>>       VIR_DOMAIN_EVENT_ID_DISK_CHANGE = 9,     /* virConnectDomainEventDiskChangeCallback */
>> +    VIR_QEMU_DOMAIN_EVENT_ID_UNKNOWN = 10,   /* virConnectDomainEventDefaultCallback */
> No.  We do not want to be adding this event type to libvirt.so.  Qemu
> events should be sent on a different channel of the RPC protocol, and
> never interfere with libvirt events.
>
>>
>>       /*
>>        * NB: this enum value will increase over time as new events are
>> diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
>> index 614ab97..0388a66 100644
>> --- a/src/conf/domain_event.c
>> +++ b/src/conf/domain_event.c
>> @@ -45,7 +45,9 @@ typedef virDomainMeta *virDomainMetaPtr;
>>
>>   struct _virDomainEventCallback {
>>       int callbackID;
>> +    int qemuCallbackID;
>>       int eventID;
>> +    char *eventName;
> Since we don't want to piggy-back main libvirt events, but instead send
> this directly from qemu, I'm wondering if it's better to have a
> qemu-specific structure in src/qemu rather than trying to combine it
> into conf/domain_event.c.
>
>>       virConnectPtr conn;
>>       virDomainMetaPtr dom;
>>       virConnectDomainEventGenericCallback cb;
>> @@ -94,6 +96,10 @@ struct _virDomainEvent {
>>               char *devAlias;
>>               int reason;
>>           } diskChange;
>> +        struct {
>> +            char *eventName;
>> +            char *eventArgs;
>> +        }qemuUnknownEvent;
> Style: space after }.
>
>> +/**
>> + * virDomainEventCallbackListAddName:
>> + * @conn: pointer to the connection
>> + * @cbList: the list
>> + * @eventName: the event eventName
>> + * @callback: the callback to add
>> + * @eventID: the specific eventID
>> + * @opaque: opaque data tio pass to callback
> s/tio/to/
>
> but again, I'm not sure we want this function in the generic libvirt
> code; it may fit better in the qemu-specific code.
>
>> + *
>> + * Internal function to add a callback from a virDomainEventCallbackListPtr
>> + */
>> +int
>> +virDomainEventCallbackListAddName(virConnectPtr conn,
>> +                                  virDomainEventCallbackListPtr cbList,
>> +                                  virDomainPtr dom,
>> +                                  const char* eventName,
>> +                                  int eventID,
>> +                                  virConnectDomainEventGenericCallback callback,
>> +                                  void *opaque,
>> +                                  virFreeCallback freecb)
>> +{
>> +    virDomainEventCallbackPtr event;
>> +    int i;
>> +
>> +    /* Check incoming */
>> +    if ( !cbList ) {
>> +        return -1;
>> +    }
>> +
>> +    /* check if we already have this callback on our list */
>> +    for (i = 0 ; i<  cbList->count ; i++) {
>> +        if (cbList->callbacks[i]->cb == VIR_DOMAIN_EVENT_CALLBACK(callback)&&
>> +            STREQ(cbList->callbacks[i]->eventName, eventName)&&
> Is a linear search wise?  Or should we be setting up a hashtable of
> event names to the appropriate struct.  After all, we don't know how
> many event names qemu might support, so the goal of O(1) hash lookup
> will make it more efficient.
>
>> +++ b/src/libvirt-qemu.c
>> @@ -36,6 +36,77 @@
>>       virReportErrorHelper(VIR_FROM_DOM, error, NULL, __FUNCTION__,       \
>>                            __LINE__, info)
>>
>> +/* Helper macros to implement VIR_DOMAIN_DEBUG using just C99.  This
>> + * assumes you pass fewer than 15 arguments to VIR_DOMAIN_DEBUG, but
>> + * can easily be expanded if needed.
> Why are we copying this pre-processor magic?  It seems like it should be
> coming from a common header, and not something repeated in this .c file.
>   If we need to move it into a common location in order to reuse it, then
> that should be a pre-requisite patch (don't mix code motion and new code
> in the same patch).
>
>>   /**
>>    * virDomainQemuMonitorCommand:
>>    * @domain: a domain object
>> @@ -178,3 +249,121 @@ error:
>>       virDispatchError(conn);
>>       return NULL;
>>   }
>> +
>> +/**
>> + * virConnectDomainQemuEventRegister:
>> + * @conn: pointer to the connection
>> + * @dom: pointer to the domain
>> + * @eventName: the event Name to receive
>> + * @cb: callback to the function handling domain events
>> + * @opaque: opaque data to pass on to the callback
>> + * @freecb: optional function to deallocate opaque when not used anymore
>> + *
>> + * Adds a callback to receive notifications of arbitrary qemu domain events
>> + * occurring on a domain.
>> + *
>> + * If dom is NULL, then events will be monitored for any domain. If dom
>> + * is non-NULL, then only the specific domain will be monitored
>> + *
>> + * Most types of event have a callback providing a custom set of parameters
>> + * for the event. When registering an event, it is thus neccessary to use
>> + * the VIR_DOMAIN_EVENT_CALLBACK() macro to cast the supplied function pointer
>> + * to match the signature of this method.
> This paragraph is not needed - we only support one type of event
> callback (return the entire JSON event details, no additional structure
> imposed by libvirt-qemu), so no casting is needed.
>
>> +++ b/src/libvirt_qemu.syms
>> @@ -19,3 +19,8 @@ LIBVIRT_QEMU_0.9.4 {
>>       global:
>>           virDomainQemuAttach;
>>   } LIBVIRT_QEMU_0.8.3;
>> +LIBVIRT_QEMU_0.9.9 {
>> +    global:
>> +        virConnectDomainQemuEventRegister;
>> +        virConnectDomainQemuEventDeregister;
>> +} LIBVIRT_QEMU_0.9.4;
> We'd want this to be LIBVIRT_QEMU_0.9.10.
>
> Overall, I think the signature looks reasonable, and I'm half inclined
> to split this patch into the new API and commit that pre-freeze, then
> worry about polishing up the implementation even if that is post-freeze
> (that is, the implementation would always fail, but we'd be committing
> to the API signature).  I'll see what I can come up with, because I'm
> interested in getting this patch series in.
>




More information about the libvir-list mailing list