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

[Anthony Liguori]

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<address@hidden>    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<address@hidden>     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
>
>