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

[Anthony Liguori <anthony@codemonkey.ws>]

On 02/16/2011 08:15 AM, Kevin Wolf wrote:
> Am 16.02.2011 14:43, schrieb Anthony Liguori:
>    
>> On 02/16/2011 02:50 AM, Kevin Wolf wrote:
>>      
>>> Am 16.02.2011 01:59, schrieb Anthony Liguori:
>>>
>>>        
>>>> On 02/15/2011 07:38 AM, Luiz Capitulino wrote:
>>>>
>>>>          
>>>>> On Tue, 15 Feb 2011 10:20:01 +0100
>>>>> Kevin Wolf<kwolf@redhat.com>    wrote:
>>>>>
>>>>>
>>>>>
>>>>>            
>>>>>> Am 14.02.2011 20:34, schrieb Anthony Liguori:
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> On 02/14/2011 12:34 PM, Luiz Capitulino wrote:
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>>>> On Mon, 14 Feb 2011 08:39:11 -0600
>>>>>>>> Anthony Liguori<anthony@codemonkey.ws>     wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> On 02/14/2011 06:45 AM, Luiz Capitulino wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>>>> So the question is: how does the schema based design support extending
>>>>>>>>>> commands or events? Does it require adding new commands/events?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      
>>>>>>>>> Well, let me ask you, how do we do that today?
>>>>>>>>>
>>>>>>>>> Let's say that I want to add a new parameter to the `change' function so
>>>>>>>>> that I can include a salt parameter as part of the password.
>>>>>>>>>
>>>>>>>>> The way we'd do this today is by checking for the 'salt' parameter in
>>>>>>>>> qdict, and if it's not present, use a random salt or something like that.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>> You likely want to do what you did before. Of course that you have to
>>>>>>>> consider if what you're doing is extending an existing command or badly
>>>>>>>> overloading it (like change is today), in this case you'll want to add
>>>>>>>> a new command instead.
>>>>>>>>
>>>>>>>> But yes, the use-case here is extending an existing command.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> However, if I'm a QMP client, how can I tell whether you're going to
>>>>>>>>> ignore my salt parameter or actually use it?  Nothing in QMP tells me
>>>>>>>>> this today.  If I set the salt parameter in the `change' command, I'll
>>>>>>>>> just get a success message.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>> I'm sorry?
>>>>>>>>
>>>>>>>> { "execute": "change", "arguments": { "device": "vnc", "target": "password", "arg": "1234", "salt": "r1" } }
>>>>>>>> {"error": {"class": "InvalidParameter", "desc": "Invalid parameter 'salt'", "data": {"name": "salt"}}}
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>> So I'm supposed to execute the command, and if execution fails, drop the
>>>>>>> new parameter?  If we add a few optional parameters, does that mean I
>>>>>>> have to try every possible combination of parameters?
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> How is that different from trying out multiple commands? In the end, you
>>>>>> always need some meta information like a schema in order to avoid trying
>>>>>> out which parameters the server supports.
>>>>>>
>>>>>> Anyway, I think there's a second interesting point: Adding parameters
>>>>>> does cause these problems, but it's different for data sent from qemu to
>>>>>> the client (return values and events). If we add more information there,
>>>>>> an older client can just ignore it, without even looking at a schema.
>>>>>>
>>>>>> So I think we should consider this for return values and definitely do
>>>>>> it for events. Sending out five different messages for a single event
>>>>>> that are completely redundant and only differ in the number of fields is
>>>>>> just insane (okay, they wouldn't actually get on the wire because a
>>>>>> client registers only for one of them, but the code for generating them
>>>>>> must exist).
>>>>>>
>>>>>>
>>>>>>              
>>>>> That's my point when I asked about events in the other thread.
>>>>>
>>>>>
>>>>>            
>>>> Okay, I had confused myself about this.  It's not quite as bad as I had
>>>> been saying.
>>>>
>>>> One of the reasons to have generated allocation function is so that we
>>>> can make sure to always pad structures.  Since all optional fields has a
>>>> bool to indicate the fields presence, by setting the allocated structure
>>>> to zero, we can support forwards compatibility for structures.
>>>>
>>>>          
>>> I think in most cases we would even get away with a default value
>>> instead of the bool. For example for strings, NULL would be a very clear
>>> indication that the field wasn't there in the JSON message.
>>>        
>> In order to support forwards compatibility, we need to have a zero-value
>> for non-presence.  For strings or pointers, NULL would work very well.
>>      
> What's the kind of compatibility you're concerned about? I was mainly
> considering older clients communicating with newer qemu, i.e.
> compatibility on the protocol level.

The has_XX fields are not sent over the wire.

>   The library can set default values
> for fields that are not present in JSON messages it receives.
>
> Your point is older applications using a newer library?

Compiled against a new library, but running against an old library.

This has to be supported in order to avoid bumping the library version.

>> That's not entirely true.   For human-monitor-command, we return a bare
>> string.  For all other commands, we return structures specifically to
>> enable better forwards compatibility.
>>
>> I do agree that for most of our events, we should be using a structure
>> for passing information.  That's not what we're doing today but there's
>> only a couple events that are even doing that so fixing it won't be that
>> bad.
>>      
> Right, you could still have something like this (although I'm not sure
> if it's very useful):
>
> [ 'block-io-event', {}, {}, 'string' ]
>
> What I think isn't a good idea is that the following definition doesn't
> generate a structure in your original proposal. This should really
> generate a structure:
>
> { 'BlockIOEvent': {'device': 'string', 'action': 'string', 'operation':
> 'string'} }
> [ 'block-io-event', {}, {}, 'BlockIOEvent' ]
>    

Actually, I think it's better to explicitly call out the structure name 
so that you can do things like:

{ 'VncConnectedEvent': {'info': 'VncClientInfo'} }

Which happens to be the same structure used by 'query-vnc'

Regards,

Anthony Liguori

> Kevin
>
>