On Mon, 14 Feb 2011 17:58:00 -0200
Luiz Capitulino <lcapitul...@redhat.com> wrote:
> On Mon, 14 Feb 2011 13:34:11 -0600
> Anthony Liguori <anth...@codemonkey.ws> wrote:
>
> > On 02/14/2011 12:34 PM, Luiz Capitulino wrote:
> > > On Mon, 14 Feb 2011 08:39:11 -0600
> > > Anthony Liguori<anth...@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?
>
> 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? :)
