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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 12 May 2010 18:48:38 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> > +query-block
>> > +-----------
>> > +
>> > +Show the block devices.
>> > +
>> > +Each block device information is stored in a json-object and the returned 
>> > value
>> > +is a json-array of all devices.
>> > +
>> > +Each json-object contain the following:
>> > +
>> > +- "device": device name (json-string)
>> > +- "type": device type (json-string)
>> 
>> Possible values?  "hd", "cdrom", "floppy".  Code also has "unknown", but
>> when it uses that, it's badly broken.
>
>  Yes, but you didn't mean we shouldn't use 'unknown', right?

Shouldn't we reply with some internal error when it happens instead of
sending a crap value?  Anyway, it's not a problem with this patch, just
a separate issue I found while reviewing it.

Documentation for the expected values would be nice.  I'm fine with
doing that in a follow-up patch.  Same for the other places I flagged.

>> > +- "removable": true if the device is removable, false otherwise 
>> > (json-bool)
>> > +- "locked": true if the device is locked, false otherwise (json-bool)
>> > +- "inserted": only present if the device is inserted, it is a json-object
>> > +   containing the following:
>> > +         - "file": device file name (json-string)
>> > +         - "ro": true if read-only, false otherwise (json-bool)
>> > +         - "drv": driver format name (json-string)
>> 
>> Possible values?
>
>  I got the following list by grepping the code. Kevin, can you confirm it's
> correct?
>
>  "blkdebug", "bochs", "cloop", "cow", "dmg", "file", "file", "ftp", "ftps",
>  "host_cdrom", "host_cdrom", "host_device", "host_device", "host_floppy",
>  "http", "https", "nbd", "parallels", "qcow", "qcow2", "raw", "tftp", "vdi",
>  "vmdk", "vpc", "vvfat"
>
>> > +query-cpus
>> > +----------
>> > +
>> > +Show CPU information.
>> > +
>> > +Return a json-array. Each CPU is represented by a json-object, which 
>> > contains:
>> > +
>> > +- "cpu": CPU index (json-int)
>> 
>> It's actually upper case (see example below).  I hate that.
>
>  Hm, this one leaked.. But it's quite minor anyway.

My comment on this patch is "please make it consistent with the code",
no more.

>> > +- "current": true if this is the current CPU, false otherwise (json-bool)
>> > +- "halted": true if the cpu is halted, false otherwise (json-bool)
>> > +- Current program counter. The key's name depends on the architecture:
>> > +     "pc": i386/x86_64 (json-int)
>> > +     "nip": PPC (json-int)
>> > +     "pc" and "npc": sparc (json-int)
>> > +     "PC": mips (json-int)
>> 
>> Key name depending on arch: isn't that an extraordinarily bad idea?
>
>  Honestly, I don't think it's that bad: it's a form of optional key,
> but if you think it's so bad I can add a "arch" key with the arch's name.
>
>  Don't think anyone is using this, anyway.

Why not call it "pc" and be done with it?

Again, this is not a problem with this patch, but a separate issue.

>> > +query-migrate
>> > +-------------
>> > +
>> > +Migration status.
>> > +
>> > +Return a json-object. If migration is active there will be another 
>> > json-object
>> > +with RAM migration status and if block migration is active another one 
>> > with
>> > +block migration status.
>> > +
>> > +The main json-object contains the following:
>> > +
>> > +- "status": migration status (json-string)
>> 
>> Possible values?  "active", "completed", "failed", "cancelled".  Note
>> there's no value for "never attempted"; see below.
>> 
>> > +- "ram": only present if "status" is "active", it is a json-object with 
>> > the
>> > +  following RAM information (in bytes):
>> > +         - "transferred": amount transferred (json-int)
>> > +         - "remaining": amount remaining (json-int)
>> > +         - "total": total (json-int)
>> > +- "disk": only present if "status" is "active" and it is a block 
>> > migration,
>> > +  it is a json-object with the following disk information (in bytes):
>> > +         - "transferred": amount transferred (json-int)
>> > +         - "remaining": amount remaining (json-int)
>> > +         - "total": total (json-int)
>> 
>> Before the first migration, we actually reply with
>> 
>> {"return": {}}
>> 
>> which contradicts the doc.
>
>  Good catch, what would be the best behavior here?

Three behaviors come to mind:

* There is no migration status before migration has been attempted, and
  asking for it is an error.  So send one.

* Invent a new value for "status".

* Pretend the (non-existant) previous migration completed.

Matter of taste.  Last one is the simplest.

Anyway, separate issue.

>> > +Example:
>> > +
>> > +{
>> > +   "return":[
>> > +      {
>> > +         "bus":0,
>> > +         "devices":[
>> > +            {
>> > +               "bus":0,
>> > +               "qdev_id":"",
>> > +               "slot":0,
>> > +               "class_info":{
>> > +                  "class":1536,
>> > +                  "desc":"Host bridge"
>> > +               },
>> > +               "id":{
>> > +                  "device":32902,
>> > +                  "vendor":4663
>> > +               },
>> > +               "function":0,
>> > +               "regions":[
>> > +
>> > +               ]
>> 
>> Suggest to simply write
>> 
>>                   "regions":[]
>
>  I could, and I agree it's better, but I'm using a formatting tool, so
> editing by hand would be a PITA.
>
>> > +Note: This example has been shortened as the real response is too long.
>> 
>> Actually, I get a shorter response for my minimal guest: just slots
>> 0..3.  Suggest to omit slot 4 and this note.
>
>  What's the command-line for it?

$ qemu-system-x86_64 -nodefaults -enable-kvm -m 1024 -vga cirrus -S -vnc :0 
-usb -readconfig ~/work/qemu/test.cfg

with the following test.cfg (created by -writeconfig):

# qemu config file

[drive "hda"]
  if = "none"
  file = "f12.img"

[chardev "monitor"]
  backend = "stdio"

[chardev "qmp"]
  backend = "socket"
  path = "f12-qmp"
  server = "on"
  wait = "off"

[device]
  driver = "ide-drive"
  drive = "hda"
  bus = "ide.0"
  unit = "0"

[device "nic.0"]
  driver = "virtio-net-pci"
  netdev = "net.0"

[netdev "net.0"]
  type = "user"

[mon "monitor"]
  mode = "readline"
  chardev = "monitor"
  default = "on"

[mon "qmp"]
  mode = "control"
  chardev = "qmp"

>> > +query-vnc
>> > +---------
>> > +
>> > +Show VNC server information.
>> > +
>> > +Return a json-object with server information. Connected clients are 
>> > returned
>> > +as a json-array of json-objects.
>> > +
>> > +The main json-object contains the following:
>> > +
>> > +- "enabled": true or false (json-bool)
>> > +- "host": server's IP address (json-string)
>> 
>> Wouldn't hurt to mention it can be a wildcard address.  The example
>> below shows the IPv4 wildcard address "0.0.0.0".
>> 
>> > +- "family": address family (json-string, "ipv4" or "ipv6")
>> 
>> inet_strfamily() can also return "unix" and "unknown".
>> 
>> By the way, we use PF_INET6, PF_INET and PF_UNIX there.  To be
>> pedantically correct, we should use AF_INET6, AF_INET and AF_LOCAL.
>> 
>> > +- "service": server's port number (json-string)
>> 
>> Why isn't this json-int?
>
>  Because getnameinfo() returns a string and I didn't bother using it,
> do you?

>From a protocol point of view, transmitting a number as a string is
wrong.  How we find the number in QEMU is an irrelevant implementation
detail.

Another separate issue.