Re: [Qemu-devel] Re: [RFC] qapi: events in QMP

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/14/2011 06:32 AM, Kevin Wolf wrote:
> Am 14.02.2011 13:03, schrieb Anthony Liguori:
>    
>> On 02/14/2011 03:50 AM, Kevin Wolf wrote:
>>      
>>> Am 13.02.2011 19:08, schrieb Anthony Liguori:
>>>        
>>>> Proposal for events in QAPI
>>>>
>>>> For QAPI, I'd like to model events on the notion of signals and
>>>> slots[2].  A client would explicitly connect to a signal through a QMP
>>>> interface which would result in a slot being added that then generates
>>>> an event.  Internally in QEMU, we could also connect slots to the same
>>>> signals.  Since we don't have an object API in QMP, we'd use a pair of
>>>> connect/disconnect functions that had enough information to identify the
>>>> signal.
>>>>
>>>> Example:
>>>>
>>>> We would define QEVENT_BLOCK_IO_EVENT as:
>>>>
>>>> # qmp-schema.json
>>>> { 'BlockIOEvent': {'device': 'str', 'action': 'str', 'operation': 'str'} }
>>>>
>>>> The 'Event' suffix is used to determine that the type is an event and
>>>> not a structure.  This would generate the following structures for QEMU:
>>>>
>>>> typedef void (BlockIOEventFunc)(const char *device, const char *action,
>>>> const char *operation, void *opaque);
>>>>
>>>>          
>>> Why is an event not a structure? For one it makes things inconsistent
>>> (you have this 'Event' suffix magic), and it's not even convenient. The
>>> parameter list of the BlockIOEventFunc might become very long. At the
>>> moment you have three const char* there and I think it's only going to
>>> grow - who is supposed to remember the right order of arguments?
>>>
>>> So why not make the event a struct and have a typedef void
>>> (BlockIOEventFunc)(BlockIOEvent *evt)?
>>>
>>>        
>> A signal is a function call.  You can pass a structure as a parameter is
>> you so desire but the natural thing to do is pass position arguments.
>>
>> If you've got a ton of signal arguments, it's probably an indication
>> that you're doing something wrong.
>>      
> Yes. For example, you're taking tons of independent arguments for
> something that is logically a single entity, namely a block error event. ;-)
>
> I'm almost sure that we'll want to add more things to this specific
> event, for example a more detailed error description (Luiz once
> suggested using errno here, but seems it hasn't made its way into
> upstream). And I would be surprised if we never wanted to add more
> information to other events, too.
>    

You can pass structures as part of events.  For instance:

{ 'BlockIOEventInfo': { 'action': 'str', 'operation': 'str' } }
{ 'BlockIOEvent': { 'data': 'BlockIOEventInfo' } }

Which generates:

typedef struct BlockIOEventInfo
{
      const char *action;
      const char *operation;
} BlockIOEventInfo;

typedef void (BlockIOEventFunc)(BlockIOEventInfo *data, void *opaque);

There are some cases where this might make sense.  For instance, device 
hotplug events might want to pass quite a bit of device info in the event.

It's just like designing any normal API.  Sometimes you need to pass 
structs and sometimes adding more args is the right thing to do.

>>    From a protocol perspective, events look like this today:
>>
>> { "event": "BlockIOError", "data": { "device": "ide1-cd0", "operation":
>> "read", "action": "report" } }
>>
>> The only change to the protocol I'm proposing is adding a "tag" field
>> which would give us:
>>
>> { "event": "BlockIOError", tag: "event023234", "data": { "device":
>> "ide1-cd0", "operation": "read", "action": "report" } }
>>      
> Right, I was more referring to this schema thing. I didn't quite
> understand yet if/why functions and events are the same thing from that
> perspective.
>
> They seem to be some kind of function that is called from within qemu,
> gets its arguments from the event_connect call and returns its return
> value to the QMP client.
>
> Is that about right?
>    

Close.  It's a function that's called from within QEMU whereas the 
arguments to the function end up being sent as the payload for the event.

The arguments to connect allow the client to connect to very specific 
signals.  For example, each BlockDriverState may have it's own 
BlockIOEvent signal.  So:

struct BlockDriverState {
      // ...
      BlockIOEvent ioevent;
};

And then the various places would do:

block_io_event_signal(&bs->ioevent, "report", "read");

The connect operation is used to figure out which bs->ioevent to connect 
to.  So it would look like:

block_io_event_connect("ide0-hd0", my_io_event_handler, &mystate);

We could eliminate the need for individual connect/disconnect operations 
if we had a global object model where you could describe objects in a 
unified hierarchy.  For instance:

signal_connect("/blockdev/ide0-hd0", my_ioevent_handler, &mystate);

But this is a pretty big change that I fear is overengineering.

>>>> Another Example
>>>>
>>>> Here's an example of BlockDeviceEvent, the successor to BlockIOEvent.
>>>> It makes use of signal accessor arguments and QAPI enumeration support:
>>>>
>>>>          
>>> So are we going to drop BlockIOEvent (or any other event that will be
>>> extended) or will both old and new version continue to exist and we
>>> always signal both of them?
>>>        
>> Both have to exist.  We can't drop old events without breaking backwards
>> compatibility.
>>      
> Right. So a second event doesn't sound like something we'd love to
> have... Maybe extending the existing ones with optional arguments would
> be better?
>    

See the other thread about using optional arguments to extend.  I think 
it's extremely hard to write a client if we use this model to do 
extensions.  It basically forces you to work with variant types instead 
of native types.

Regards,

Anthony Liguori

> Kevin
>
>