On Sun, 22 Nov 2009 11:41:02 +0200
Avi Kivity <a...@redhat.com> wrote:
> On 11/19/2009 06:47 PM, Luiz Capitulino wrote:
> >
> >> Can you post a capture of a few monitor commands through the new protocol?
> >>
> > Here goes, it's a telnet session:
> >
>
> Looks really good, some comments below.
>
> > """
> > {"QMP": {"capabilities": []}}
> >
> > { "execute": "info", "arguments": { "item": "balloon" } }
> > {"return": 1024}
> >
>
> 1. I see no id attribute, but it's supported, yes?
Yes, it can be any json-value, like this real example:
{ "execute": "info", "arguments": { "item": "balloon" }, "id": "foobar" }
{"return": 1024, "id": "foobar"}
> 2. I asked before for info commands to be separated into individual
> commands ("query-balloon") when in machine mode. You wouldn't write a
> function info(enum info_thing what), would you? What would its return
> type be?
Yes, you're right.. I didn't do it yet because I was focusing in getting
the protocol working first.
There is an easy way to support it though. We could add a check for
commands which start with query- and call do_info() with the
appropriate arguments.
This way we get this syntax valid for the first merge and postpone the
Monitor refactoring to properly support it to 0.13.
Actually, the Monitor needs a big refactoring and that was true
even before QMP.
> 3. Quantities, for the machine protocol, should be in natural units (in
> this case, bytes). The human interface can use kMGT and have some
> reasonable default.
Will fix.
> > { "execute": "info", "arguments": { "item": "balloon" } }
> > {"return": 512}
> >
> > { "execute": "info", "arguments": { "item": "network" } }
> > {"return": [{"devices": [{"name": "user.0", "info": "net=10.0.2.0,
> > restricted=n"}, {"name": "e1000.0", "info":
> > "model=e1000,macaddr=52:54:00:12:34:56"}], "id": 0}]}
> >
>
> The internal "info" is very worrying. We need to make sure everything
> is returned as an object without the need for additional parsing.
That's the complicated part.
Some (several?) Monitor commands would require deeper changes to return
100% of their information in QObject style.
This takes time for me, as I have to dig in subsystems I'm not so
familiar with and have to work on the other series too.
So, I've chosen to do the easy conversion now so that the project
goes forward. I know this can compromise protocol stability, but I was
planning not to declare the protocol stable for 0.12.
This is a discussion we should have.
> > { "execute": "migrate", "arguments": { "detach": "-d", "uri":
> > "tcp:localhost:4445" } }
> > {"error": {"class": "InvalidParameter", "data": {"parameter": "detach",
> > "reason": "must be an integer"}}}
> >
>
> I presume true and false would work here?
Yes, at the time I introduced QDict I couldn't foresee that.
