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

[Anthony Liguori <anthony@codemonkey.ws>]

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.

But for integers, I'm not sure we can reasonably use 0 as a universal 
default value.  We could just use has_ fields for non-pointers but I 
figured consistency would make the code more robust since it's hard to 
tell that a field is really optional vs. required.  For instance:

typedef struct BlockInfo {
     const char *device_name;
     bool has_backing_file;
     const char *backing_file;
} BlockInfo;

It's very obvious that backing_file is optional.  If you don't set 
has_backing_file (because you're incorrectly treating backing_file is 
required), it will fail immediately as the field won't be marshalled.

OTOH:

typedef struct BlockInfo {
      const char *device_name;
      // optional
      const char *backing_file;
} BlockInfo;

Is a bit easier to screw up.  If you happen to not do the NULL check and 
work with a client that's sending it, you can end up with a NULL pointer 
dereference pretty easily.

>> If we expect to add fields later, we just have to make sure we use a
>> structure to encapsulate things.
>>      
> As stated before, I think we should use structures for all events. I
> still don't understand why we should have an exception for events. Any
> other command returns structures, too, and you don't automagically pull
> their fields up one level anywhere except for events.
>    

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.

Regards,

Anthony Liguori

> Kevin
>
>