Re: [Qemu-devel] [PATCH 1/2] QMP: Introduce commands doc

[Luiz Capitulino]

On Fri, 14 May 2010 10:39:29 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Thu, 13 May 2010 16:48:13 +0300
> > Avi Kivity <address@hidden> wrote:
> >
> >> On 05/05/2010 10:11 PM, Luiz Capitulino wrote:
> >> > One of the most important missing feature in QMP today is its
> >> > supported commands documentation.
> >> >
> >> > The plan is to make it part of self-description support, however
> >> > self-description is a big task we have been postponing for a
> >> > long time now and still don't know when it's going to be done.
> >> >
> >> > In order not to compromise QMP adoption and make users' life easier,
> >> > this commit adds a simple text documentation which fully describes
> >> > all QMP supported commands.
> >> >
> >> > This is not ideal for a number of reasons (harder to maintain,
> >> > text-only, etc) but does improve the current situation.
> [...]
> >> > +pmemsave
> >> > +--------
> >> > +
> >> > +Save to disk physical memory dump starting at 'val' of size 'size'.
> >> > +
> >> > +Arguments:
> >> > +
> >> > +- "val": the starting address (json-int)
> >> >    
> >> 
> >> Why "val" instead of "address" or "physical-address"?
> >
> >  Mea culpa, for this and most inconsistencies you find in the protocol.
> >
> >  The reason is that I did a lot of conversions in a hurry, so that
> > we could get libvirt working under QMP as soon as possible.
> >
> >  Unfortunately, some bad names and some bad behaviors from the user
> > Monitor leaked into QMP. Lesson learned, although sometimes it's very
> > difficult to define what a name/behavior should be in QMP.
> 
> Once we declare QMP stable, we're stuck with bad names forever.  So
> better fix them before we stabilize.  Best to fix them all in one go,
> and prepare the (hopefully straightforward) libvirt patch to go with it.

 I think we have three kinds of things to be fixed. If we do it right
they will cause breakage to libvirt, but sometimes it's possible to
fix them in a compatible way:

1) Simple renames
2) Bad user Monitor commands exposed in QMP
3) Bad/incomplete design decisions

 Doing 1) is easy, you have been fixing 2) for some commands, although
I'm not sure what we should do for do_change().

 Now, 3) concerns me more. We have the QError stuff and events. What
concerns me is that we're likely going to work on this in hurry again..

 The problem with events is that today, they look like this:

{ "event": "VNC_CONNECTED",
    "data": {
        "server": { "auth": "sasl", "family": "ipv4",
                    "service": "5901", "host": "0.0.0.0" },
        "client": { "family": "ipv4", "service": "58425",
                    "host": "127.0.0.1" } },
    "timestamp": { "seconds": 1262976601, "microseconds": 975795 } }

 Which means that a subsystem/driver/whatever which needs to report
multiple 'actions', will have to do so through multiple events. For
example, vnc has: VNC_CONNECTED, VNC_DISCONNECTED and VNC_INITIALIZED.

 A simple solution would be to add a 'subsystem' member, like:

{ "event": "CONNECTED", "subsystem": "vnc" }

 But this has a number of small problems too, like a specific driver
is not a "subsystem" and what's the subsystem for events like SHUTDOWN?

 Anthony has suggested integrating with qdev, which is something I
think we should do, but alone it doesn't fix the problem explained above.

 We had this discussion when the vnc events were introduced, but we
decided to wait for other events to think better about this. Events
have grown a little already and we have spice and migration in the queue..