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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/14/2011 03:50 AM, Kevin Wolf wrote:
> Am 13.02.2011 19:08, schrieb Anthony Liguori:
>    
>> Hi,
>>
>> In my QAPI branch[1], I've now got almost every existing QMP command
>> converted with (hopefully) all of the hard problems solved.  There is
>> only one remaining thing to attack before posting for inclusion and
>> that's events.  Here's my current thinking about what to do.
>>      
> I've had a quick look at your branch and I have one general question: Is
> there a specific reason why you duplicate existing monitor handlers
> instead of converting the existing ones? Are the old ones still used?
>    

It's just temporary.  It makes it very easy to test code side-by-side.   
By the time I submit, I'll remove the old code.

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

>> typedef struct BlockIOEvent {
>>       /* contents are private */
>> } BlockIOEvent;
>>
>> /* Connect a slot to a BlockIOEvent signal and return a handle to the
>> connection */
>> int block_io_event_connect(BlockIOEvent *event, BlockIOEventFunc *cb,
>> void *opaque);
>>
>> /* Signal any connect slots of a BlockIOEvent */
>> void block_io_event_signal(BlockIOEvent *event, const char *device,
>> const char *action, const char *operation);
>>
>> /* Disconnect a slot from a signal based on a connection handle */
>> void block_io_event_disconnect(BlockIOEvent *event, int handle);
>>
>> In the case of BlockIOEvent, this is a global signal that is not tied to
>> any specific object so there would be a global BlockIOEvent object
>> within QEMU.
>>
>>   From a QMP perspective, we would have the following function in the schema:
>>
>> [ 'block-io-event', {}, {}, 'BlockIOEvent' ]
>>      
> Not sure what this list is supposed to tell me...
>
> I see that you use the same for command, so I guess the first one is
> something like an command/event name, the second seems to be the
> arguments, last could be the type of the return value, the third no idea.'
>    

Sorry, first is the name of the function, second is required arguments, 
third is optional arguments, last is return value.

> So would an event look like a response to a command that was never issued?
>    

 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" } }

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

Regards,

Anthony Liguori

> Kevin
>
>