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

[Luiz Capitulino]

On Mon, 14 Feb 2011 17:58:00 -0200
Luiz Capitulino <address@hidden> wrote:

> On Mon, 14 Feb 2011 13:34:11 -0600
> Anthony Liguori <address@hidden> wrote:
> 
> > 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?
> 
> No, of course not, our plan has always been to do this via an schema,
> the only reason we don't do this today is lack of time/help.
> 
> > >> Even if we expose a schema, but leave things as-is, having to parse the
> > >> schema as part of a function call is pretty horrible,
> > >>      
> > > That's a client implementation detail, they are not required to do it
> > > as part of a function call.
> > >
> > > But let me ask, if we don't expose a schema, how will clients be able to
> > > query available commands/events and their parameters?
> > >    
> > 
> > We need to expose the schema, I'm not saying we shouldn't.  But we don't 
> > today.
> > 
> > You're arguing that we should extend commands by adding new parameters.  
> 
> Commands and events, you haven't commented on events yet and that seems
> a bit worse than commands.
> 
> > I'm saying that's a bad interface.  If we need to change a command, we 
> > should introduce a new command.  It's a well understood mechanism for 
> > maintaining compatibility (just about every C library does exactly this).
> 
> So, let's agree we disagree.
> 
> > >> particularly if
> > >> distros do silly things like backport some optional parameters and not
> > >> others.  If those optional parameters are deeply nested in a structure,
> > >> it's even worse.
> > >>      
> > > Why would they do this? I mean, if distros (or anyone else shipping qemu)
> > > goes that deep on changing the wire protocol they are on their own, why
> > > would we want to solve this problem?
> > >    
> > 
> > It's not at all unreasonable for a distro to backport a new QMP 
> > command.  If all modifications are discrete commands, compatibility is 
> > easy to preserve, however if a distro does backporting and we end up 
> > with a frankenstein command, compatibility will be an issue.
> 
> I disagree. Let's say we have added three new arguments to the command foo,
> and now we have foo1, foo2 and foo3. I'm a quite old distro and only
> have foo, which command should I backport? All of them? Only the latest?
> 
> I can't see how easier this is. Backporting APIs will almost always suck.
> 
> > >> OTOH, if we introduce a new command to set the password with a salt, it
> > >> becomes very easy for the client to support.  The do something as simple 
> > >> as:
> > >>
> > >> if qmp.has_command("vnc-set-password-with-salt"):
> > >>       qmp.vnc_set_password_with_salt('foobar', 'X*')
> > >> else:
> > >>       window.set_weak_security_icon(True)
> > >>       qmp.vnc_set_password('foobar')
> > >>
> > >> Now you could answer, hey, we can add capabilities then those
> > >> capabilities can quickly get out of hand.
> > >>      
> > > Adding one command per new argument has its problems too and it's even
> > > worse with events, as clients will have to be changed to handle a
> > > new event just because of a parameter addition.
> > >    
> > 
> > Yes, but it's an extremely well understood way to design compatible APIs.
> 
> For C, yes. But one of the main goals of a high level protocol is to be
> language independent, isn't it?
> 
> > > Look, although I did _not_ check any code yet, your description of the 
> > > QAPI
> > > looks really exciting. I'm not against it, what bothers me though is this
> > > number of small limitations we're imposing to the wire protocol.
> > >
> > > Why don't we make libqmp internal only? This way we're free to change it
> > > whatever we want.
> > >    
> > 
> > libqmp is a test of how easy it is to use QMP from an external 
> > application.  If we can't keep libqmp stable, then that means tools like 
> > libvirt will always have a hard time using QMP.
> > 
> > Proper C support is important.  We cannot make it impossible to write a 
> > useful C client API.
> 
> I wouldn't say it's impossible, but anyway, the important point here is
> that we disagree about the side effects QAPI is going to introduce in QMP,
> I don't know how to solve this, maybe we can discuss this upstream, but I'm
> not sure the situation will change much.

Oh, it's upstream, let's vote? :)