[1/2] QMP: Introduce commands doc

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.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/README           |    5 +-
 QMP/qmp-commands.txt | 1033 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 1036 insertions(+), 2 deletions(-)
 create mode 100644 QMP/qmp-commands.txt

Luiz Capitulino <lcapitulino@redhat.com> writes:

> 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.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/README           |    5 +-
>  QMP/qmp-commands.txt | 1033 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 1036 insertions(+), 2 deletions(-)
>  create mode 100644 QMP/qmp-commands.txt
>
> diff --git a/QMP/README b/QMP/README
> index 9334c25..35a80c7 100644
> --- a/QMP/README
> +++ b/QMP/README
> @@ -15,8 +15,9 @@ QMP is JSON[1] based and has the following features:
>  
>  For more information, please, refer to the following files:
>  
> -o qmp-spec.txt    QEMU Monitor Protocol current specification
> -o qmp-events.txt  List of available asynchronous events
> +o qmp-spec.txt      QEMU Monitor Protocol current specification
> +o qmp-commands.txt  QMP supported commands
> +o qmp-events.txt    List of available asynchronous events
>  
>  There are also two simple Python scripts available:
>  
> diff --git a/QMP/qmp-commands.txt b/QMP/qmp-commands.txt
> new file mode 100644
> index 0000000..b1b0e02
> --- /dev/null
> +++ b/QMP/qmp-commands.txt
> @@ -0,0 +1,1033 @@

I reviewed this part before, and it looks good now.

[...]
> +2. Query Commands
> +=================
> +
> +query-balloon
> +-------------
> +
> +Show balloon information.
> +
> +Make an asynchronous request for balloon info. When the request completes a
> +json-object will be returned containing the following data:
> +
> +- "actual": current balloon value in bytes (json-int)
> +- "mem_swapped_in": Amount of memory swapped in bytes (json-int, optional)
> +- "mem_swapped_out": Amount of memory swapped out in bytes (json-int, optional)
> +- "major_page_faults": Number of major faults (json-int, optional)
> +- "minor_page_faults": Number of minor faults (json-int, optional)
> +- "free_mem":Total amount of free and unused memory in bytes (json-int,optional)

Nitpick: space after comma, please.

> +- "total_mem": Total amount of available memory in bytes (json-int, optional)
> +
> +Example:
> +
> +{
> +   "return":{
> +      "actual":1073741824,
> +      "mem_swapped_in":0,
> +      "mem_swapped_out":0,
> +      "major_page_faults":142,
> +      "minor_page_faults":239245,
> +      "free_mem":1014185984,
> +      "total_mem":1044668416
> +   }
> +}
> +
> +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.

> +- "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?

> +         - "backing_file": backing file name if one is used (json-string)

Suggest

            - "backing_file": backing file name (json-string, optional)

for consistency with optional members elsewhere.

> +         - "encrypted": true if encrypted, false otherwise (json-bool)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "device":"ide0-hd0",
> +         "type":"hd",
> +         "removable":false,
> +         "locked":false,
> +         "inserted":{
> +            "file":"/tmp/foobar",
> +            "ro":false,
> +            "drv":"qcow2"

"encrypted" is missing here.

> +         }
> +      },
> +      {
> +         "device":"floppy0",
> +         "type":"floppy",
> +         "removable":true,
> +         "locked":false
> +      }
> +   ]
> +}
> +
> +query-blockstats
> +----------------
> +
> +Show block device statistics.
> +
> +Each device statistic 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)
> +- "stats": A json-object with the statistics information, it contains:
> +    - "rd_bytes": bytes read (json-int)
> +    - "wr_bytes": bytes written (json-int)
> +    - "rd_operations": read operations (json-int)
> +    - "wr_operations": write operations (json-int)
> +    - "wr_highest_offset": Highest offset of a sector written since the
> +                           BlockDriverState has been opened (json-int)
> +    - "parent": Contains recursively the statistics of the underlying
> +                protocol (e.g. the host file for a qcow2 image). If there is
> +                no underlying protocol, this field is omitted
> +                (json-object, optional)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "device":"ide0-hd0",
> +         "stats":{
> +            "rd_bytes":512,
> +            "wr_bytes":0,
> +            "rd_operations":1,
> +            "wr_operations":0,
> +            "wr_highest_offset":0,
> +            "parent":{
> +               "stats":{
> +                  "rd_bytes":1024,
> +                  "wr_bytes":0,
> +                  "rd_operations":2,
> +                  "wr_operations":0,
> +                  "wr_highest_offset":0
> +               }
> +            }
> +         }
> +      },
> +      {
> +         "device":"ide1-cd0",
> +         "stats":{
> +            "rd_bytes":0,
> +            "wr_bytes":0,
> +            "rd_operations":0,
> +            "wr_operations":0,
> +            "wr_highest_offset":0
> +         }
> +      }
> +   ]
> +}
> +
> +query-chardev
> +-------------
> +
> +Each device is represented by a json-object. The returned value is a json-array
> +of all devices.
> +
> +Each json-object contain the following:
> +
> +- "label": device's label (json-string)
> +- "filename": device's file (json-string)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "label":"monitor",
> +         "filename":"stdio"
> +      },
> +      {
> +         "label":"serial0",
> +         "filename":"vc"
> +      }
> +   ]
> +}
> +
> +query-commands
> +--------------
> +
> +List QMP available commands.
> +
> +Each command is represented by a json-object, the returned value is a json-array
> +of all commands.
> +
> +Each json-object contain:
> +
> +- "name": command's name (json-string)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "name":"query-balloon"
> +      },
> +      {
> +         "name":"system_powerdown"
> +      }
> +   ]
> +}
> +
> +Note: This example has been shortened as the real response is too long.
> +
> +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.

> +- "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?

> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "CPU":0,
> +         "current":true,
> +         "halted":false,
> +         "pc":3227107138
> +      },
> +      {
> +         "CPU":1,
> +         "current":false,
> +         "halted":true,
> +         "pc":7108165
> +      }
> +   ]
> +}
> +
> +query-hpet
> +----------
> +
> +Show HPET state.
> +
> +Return a json-object with the following information:
> +
> +- "enabled": true if hpet if enabled, false otherwise (json-bool)
> +
> +Example:
> +
> +{ "return": { "enabled": true } }
> +
> +query-kvm
> +---------
> +
> +Show KVM information.
> +
> +Return a json-object with the following information:
> +
> +- "enabled": true if KVM support is enabled, false otherwise (json-bool)
> +- "present": true if QEMU has KVM support, false otherwise (json-bool)
> +
> +Example:
> +
> +{ "return": { "enabled": true, "present": true } }
> +
> +query-mice
> +----------
> +
> +Show VM mice information.
> +
> +Each mouse is represented by a json-object, the returned value is a json-array
> +of all mice.
> +
> +The mouse json-object contains the following:
> +
> +- "name": mouse's name (json-string)
> +- "index": mouse's index (json-int)
> +- "current": true if this mouse is receiving events, false otherwise (json-bool)
> +- "absolute": true if the mouse generates absolute input events (json-bool)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "name":"QEMU Microsoft Mouse",
> +         "index":0,
> +         "current":false,
> +         "absolute":false
> +      },
> +      {
> +         "name":"QEMU PS/2 Mouse",
> +         "index":1,
> +         "current":true,
> +         "absolute":true
> +      }
> +   ]
> +}
> +
> +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.

> +
> +Examples:
> +
> +1. Migration is "completed":
> +
> +{ "return": { "status": "completed" } }
> +
> +2. Migration is "active" and it is not a block migration:
> +
> +{
> +   "return":{
> +      "status":"active",
> +      "ram":{
> +         "transferred":123,
> +         "remaining":123,
> +         "total":246
> +      }
> +   }
> +}
> +
> +3. Migration is "active" and it is a block migration:
> +
> +{
> +   "return":{
> +      "status":"active",
> +      "ram":{
> +         "total":1057024,
> +         "remaining":1053304,
> +         "transferred":3720
> +      },
> +      "disk":{
> +         "total":20971520,
> +         "remaining":20880384,
> +         "transferred":91136
> +      }
> +   }
> +}
> +
> +query-name
> +----------
> +
> +Show VM name.
> +
> +Return a json-object with the following information:
> +
> +- "name": VM's name (json-string, optional)
> +
> +Example:
> +
> +{ "return": { "name": "qemu-name" } }
> +
> +query-pci
> +---------
> +
> +PCI buses and devices information.
> +
> +The returned value is a json-array of all buses. Each bus is represented by
> +a json-object, which has a key with a json-array of all PCI devices attached
> +to it. Each device is represented by a json-object.
> +
> +The bus json-object contains the following:
> +
> +- "bus": bus number (json-int)
> +- "devices": a json-array of json-objects, each json-object represents a
> +             PCI device
> +
> +The PCI device json-object contains the following:
> +
> +- "bus": identical to the parent's bus number (json-int)
> +- "slot": slot number (json-int)
> +- "function": function number (json-int)
> +- "class_info": a json-object containing:
> +     - "desc": device class description (json-string, optional)
> +     - "class": device class number (json-int)
> +- "id": a json-object containing:
> +     - "device": device ID (json-int)
> +     - "vendor": vendor ID (json-int)
> +- "irq": device's IRQ if assigned (json-int, optional)
> +- "qdev_id": qdev id string (json-string)
> +- "pci_bridge": It's a json-object, only present if this device is a
> +                PCI bridge, contains:
> +     - "bus": bus number (json-int)
> +     - "secondary": secondary bus number (json-int)
> +     - "subordinate": subordinate bus number (json-int)
> +     - "io_range": a json-object with memory range information (json-int)
> +     - "memory_range": a json-object with memory range information (json-int)
> +     - "prefetchable_range": a json-object with memory range
> +                             information (json-int)
> +     - "devices": a json-array of PCI devices if there's any attached (optional)
> +- "regions": a json-array of json-objects, each json-object represents a
> +             memory region of this device
> +
> +The memory range json-object contains the following:
> +
> +- "base": base memory address (json-int)
> +- "limit": limit value (json-int)
> +
> +The region json-object can be an I/O region or a memory region, an I/O region
> +json-object contains the following:
> +
> +- "type": "io" (json-string, fixed)
> +- "bar": BAR number (json-int)
> +- "address": memory address (json-int)
> +- "size": memory size (json-int)
> +
> +A memory region json-object contains the following:
> +
> +- "type": "memory" (json-string, fixed)
> +- "bar": BAR number (json-int)
> +- "address": memory address (json-int)
> +- "size": memory size (json-int)
> +- "mem_type_64": true or false (json-bool)
> +- "prefetch": true or false (json-bool)
> +
> +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":[]


> +            },
> +            {
> +               "bus":0,
> +               "qdev_id":"",
> +               "slot":1,
> +               "class_info":{
> +                  "class":1537,
> +                  "desc":"ISA bridge"
> +               },
> +               "id":{
> +                  "device":32902,
> +                  "vendor":28672
> +               },
> +               "function":0,
> +               "regions":[
> +
> +               ]
> +            },
> +            {
> +               "bus":0,
> +               "qdev_id":"",
> +               "slot":1,
> +               "class_info":{
> +                  "class":257,
> +                  "desc":"IDE controller"
> +               },
> +               "id":{
> +                  "device":32902,
> +                  "vendor":28688
> +               },
> +               "function":1,
> +               "regions":[
> +                  {
> +                     "bar":4,
> +                     "size":16,
> +                     "address":49152,
> +                     "type":"io"
> +                  }
> +               ]
> +            },
> +            {
> +               "bus":0,
> +               "qdev_id":"",
> +               "slot":2,
> +               "class_info":{
> +                  "class":768,
> +                  "desc":"VGA controller"
> +               },
> +               "id":{
> +                  "device":4115,
> +                  "vendor":184
> +               },
> +               "function":0,
> +               "regions":[
> +                  {
> +                     "prefetch":true,
> +                     "mem_type_64":false,
> +                     "bar":0,
> +                     "size":33554432,
> +                     "address":4026531840,
> +                     "type":"memory"
> +                  },
> +                  {
> +                     "prefetch":false,
> +                     "mem_type_64":false,
> +                     "bar":1,
> +                     "size":4096,
> +                     "address":4060086272,
> +                     "type":"memory"
> +                  },
> +                  {
> +                     "prefetch":false,
> +                     "mem_type_64":false,
> +                     "bar":6,
> +                     "size":65536,
> +                     "address":-1,
> +                     "type":"memory"
> +                  }
> +               ]
> +            },
> +            {
> +               "bus":0,
> +               "qdev_id":"",
> +               "irq":11,
> +               "slot":4,
> +               "class_info":{
> +                  "class":1280,
> +                  "desc":"RAM controller"
> +               },
> +               "id":{
> +                  "device":6900,
> +                  "vendor":4098
> +               },
> +               "function":0,
> +               "regions":[
> +                  {
> +                     "bar":0,
> +                     "size":32,
> +                     "address":49280,
> +                     "type":"io"
> +                  }
> +               ]
> +            }
> +         ]
> +      }
> +   ]
> +}
> +
> +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.

> +
> +query-status
> +------------
> +
> +Return a json-object with the following information:
> +
> +- "running": true if the VM is running, or false if it is paused (json-bool)
> +- "singlestep": true if the VM is in single step mode,
> +                false otherwise (json-bool)
> +
> +Example:
> +
> +{ "return": { "running": true, "singlestep": false } }
> +
> +query-uuid
> +----------
> +
> +Show VM UUID.
> +
> +Return a json-object with the following information:
> +
> +- "UUID": Universally Unique Identifier (json-string)
> +
> +Example:
> +
> +{ "return": { "UUID": "550e8400-e29b-41d4-a716-446655440000" } }
> +
> +query-version
> +-------------
> +
> +Show QEMU version.
> +
> +Return a json-object with the following information:
> +
> +- "qemu": QEMU's version (json-string)
> +- "package": package's version (json-string)
> +
> +Example:
> +
> +{ "return": { "qemu": "0.11.50", "package": "" } }
> +
> +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?

> +- "auth": authentication method (json-string)

Possible values?  They come from vnc_auth_name().

> +- "clients": a json-array of all connected clients
> +
> +Clients are described by a json-object, each one contain the following:
> +
> +- "host": client's IP address (json-string)
> +- "family": address family (json-string, "ipv4" or "ipv6")

Same as above.

> +- "service": client's port number (json-string)

Same as above.

> +- "x509_dname": TLS dname (json-string, optional)
> +- "sasl_username": SASL username (json-string, optional)
> +
> +Example:
> +
> +{
> +   "return":{
> +      "enabled":true,
> +      "host":"0.0.0.0",
> +      "service":"50402",
> +      "auth":"vnc",
> +      "family":"ipv4",
> +      "clients":[
> +         {
> +            "host":"127.0.0.1",
> +            "service":"50401",
> +            "family":"ipv4"
> +         }
> +      ]
> +   }
> +}

On Wed, 12 May 2010 18:48:38 +0200
Markus Armbruster <armbru@redhat.com> 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?

> > +- "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.

> > +- "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.

> > +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?

> > +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?

> > +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?

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Wed, 12 May 2010 18:48:38 +0200
> Markus Armbruster <armbru@redhat.com> 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.

On Thu, 13 May 2010 09:01:21 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Wed, 12 May 2010 18:48:38 +0200
> > Markus Armbruster <armbru@redhat.com> 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.

 Separate issue, indeed, but it's good we're talking about them in this
thread.

 I'm not sure returning an error is reasonable, we should only do that
if this invalidates the whole call.

> 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.

 Good!

> >> > +- "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.

 Right.

> >> > +- "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?

 Because the interpretation of 'pc' depends on the arch, the printing
code is an example of that.

> 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.

 It's not an error, nothing went wrong.

> * Invent a new value for "status".
> 
> * Pretend the (non-existant) previous migration completed.
> 
> Matter of taste.  Last one is the simplest.

 I like the second best, but I'm not sure. Maybe migration people
should help here, as this can be improved in the user monitor
as well.

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.
>
>    

Excellent.

> +change
> +------
> +
> +Change a removable medium or VNC configuration.
>    

This is sad.  Would anyone write a C function with a similar description?

Do we have this in 0.12?  If not we can change (...) it.  If we do, it's 
a more difficult deprecation process.

> +cpu
> +---
> +
> +Set the default CPU.
> +
> +Arguments:
> +
> +- "index": the CPU's index (json-int)
> +
> +Example:
> +
> +{ "execute": "cpu", "arguments": { "index": 0 } }
> +
> +Note: CPUs' indexes are obtained with the 'query-cpus' command.
>    

The default cpu is local to the monitor?

Not sure, but it may have been better to omit it and instead require an 
explicit cpu argument in all commands that rely on it.  As it is, 
multithreaded management programs will need to lock the monitor when 
reading cpu registers, etc.

Or, we can add an optional cpu argument to all commands that depend on it.

> +
> +memsave
> +-------
> +
> +Save to disk virtual memory dump starting at 'val' of size 'size'.
> +
> +Arguments:
> +
> +- "val": the starting address (json-int)
> +- "size": the memory size (json-int)
> +- "filename": file path (json-string)
> +
> +Example:
> +
> +{ "execute": "memsave",
> +             "arguments": { "val": 10,
> +                            "size": 100,
> +                            "filename": "/tmp/virtual-mem-dump" } }
>    

Suggest a note that this depends on the current cpu.

> +
> +migrate
> +-------
> +
> +Migrate to URI.
> +
> +Arguments:
> +
> +- "blk": block migration, full disk copy (json-bool, optional)
> +- "inc": incremental disk copy (json-bool, optional)
>    

Don't these need to be per-disk?

> +- "uri": Destination URI (json-string)
> +
> +Example:
> +
> +{ "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> +
> +Notes:
> +
> +(1) The 'query-migrate' command should be used to check migration's progress
> +    and final result (this information is provided by the 'status' member)
>    

Is there an event the provides completion notification?  I don't see one 
in qemu-events.txt, but I think there should be one.

> +(2) All boolean arguments default to false
> +(3) The user Monitor's "detach" argument is invalid in QMP and should not
> +    be used
> +
> +
> +migrate_set_downtime
> +--------------------
> +
> +Set maximum tolerated downtime (in seconds) for migrations.
> +
> +Arguments:
> +
> +- "value": maximum downtime (json-number)
> +
> +Example:
> +
> +{ "execute": "migrate_set_downtime", "arguments": { "value": 60 } }
>    

The example doesn't match reality well, suggest 0.1.

Would have been nicer as migrate_set_parameters downtime: 0.1, we can do 
that when we add more parameters.

> +
> +migrate_set_speed
> +-----------------
> +
> +Set maximum speed for migrations.
> +
> +Arguments:
> +
> +- "value": maximum speed (json-number)
> +
> +Example:
> +
> +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>    

Oh, we do have more.

Please document the units for this value (bits per second)?

> +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"?

> +- "size": the memory size (json-int)
>    

In bytes.

> +- "filename": file path (json-string)
> +
> +Example:
> +
> +{ "execute": "pmemsave",
> +             "arguments": { "val": 10,
> +                            "size": 100,
> +                            "filename": "/tmp/physical-mem-dump" } }
>    

It would be nice to have non-file versions of memsave and pmemsave.

> +
> +screendump
> +----------
> +
> +Save screen into PPM image.
> +
> +Arguments:
> +
> +- "filename": file path (json-string)
> +
> +Example:
> +
> +{ "execute": "screendump", "arguments": { "filename": "/tmp/image" } }
>    

Wish:  , "format": "png"

> +query-blockstats
> +----------------
> +
> +Show block device statistics.
> +
> +Each device statistic information is stored in a json-object and the returned
> +value is a json-array of all devices.
>    

Future enhancement: request a summary, or stats for a list of devices.

> +
> +query-commands
> +--------------
> +
> +List QMP available commands.
> +
> +Each command is represented by a json-object, the returned value is a json-array
> +of all commands.
> +
> +Each json-object contain:
> +
> +- "name": command's name (json-string)
> +
> +Example:
> +
> +{
> +   "return":[
> +      {
> +         "name":"query-balloon"
> +      },
> +      {
> +         "name":"system_powerdown"
> +      }
> +   ]
> +}
> +
> +Note: This example has been shortened as the real response is too long.
>    

Presumably, this will be extended with a description of the arguments 
and return types?

> +
> +query-cpus
> +----------
> +
> +Show CPU information.
> +
> +Return a json-array. Each CPU is represented by a json-object, which contains:
> +
> +- "cpu": CPU index (json-int)
> +- "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)
>    

That's a bit sad.  It makes it difficult to use with stub generators.

Can we force everything to "pc"?  It will break non-x86, but there 
aren't likely to be many QMP users using for those archs at present.

> +
> +query-mice
> +----------
> +
> +Show VM mice information.
> +
> +Each mouse is represented by a json-object, the returned value is a json-array
> +of all mice.
> +
> +The mouse json-object contains the following:
> +
> +- "name": mouse's name (json-string)
> +- "index": mouse's index (json-int)
> +- "current": true if this mouse is receiving events, false otherwise (json-bool)
>    

What does this mean?

> +- "absolute": true if the mouse generates absolute input events (json-bool)
> +
>
> +
> +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)
>    

Please list all possible values here (what's the status before the first 
migration is started?

> +query-pci
> +---------
> +
> +PCI buses and devices information.
> +
> +The returned value is a json-array of all buses. Each bus is represented by
> +a json-object, which has a key with a json-array of all PCI devices attached
> +to it. Each device is represented by a json-object.
> +
> +The bus json-object contains the following:
> +
> +- "bus": bus number (json-int)
> +- "devices": a json-array of json-objects, each json-object represents a
> +             PCI device
> +
> +The PCI device json-object contains the following:
> +
> +- "bus": identical to the parent's bus number (json-int)
> +- "slot": slot number (json-int)
> +- "function": function number (json-int)
>    

Would have been nicer as a nested object (list of buses, each containing 
a list of slots, each containing a list of functions).

> +- "class_info": a json-object containing:
> +     - "desc": device class description (json-string, optional)
> +     - "class": device class number (json-int)
> +- "id": a json-object containing:
> +     - "device": device ID (json-int)
> +     - "vendor": vendor ID (json-int)
> +- "irq": device's IRQ if assigned (json-int, optional)
>    

Please note here that the OS may ignore it, so may not be authoritative.

> +- "qdev_id": qdev id string (json-string)
> +- "pci_bridge": It's a json-object, only present if this device is a
> +                PCI bridge, contains:
> +     - "bus": bus number (json-int)
> +     - "secondary": secondary bus number (json-int)
> +     - "subordinate": subordinate bus number (json-int)
> +     - "io_range": a json-object with memory range information (json-int)
>    

Object or int?

> +     - "memory_range": a json-object with memory range information (json-int)
>    
Object or int?

> +     - "prefetchable_range": a json-object with memory range
> +                             information (json-int)
>    

Format not understood.

> +     - "devices": a json-array of PCI devices if there's any attached (optional)
>    

What is the format of an array element?

Very nice overall.  It is *way* easier to review documentation than 
piles of code, so I suggest starting with documentation from now on.  
I've adopted this approach in kvm.

On Thu, 13 May 2010 16:48:13 +0300
Avi Kivity <avi@redhat.com> 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.
> >
> >    
> 
> Excellent.

 Thanks.

> > +change
> > +------
> > +
> > +Change a removable medium or VNC configuration.
> >    
> 
> This is sad.  Would anyone write a C function with a similar description?

 I wouldn't, but someone did for qemu.

> Do we have this in 0.12?  If not we can change (...) it.  If we do, it's 
> a more difficult deprecation process.

 Yes, we do and it's used by libvirt iirc.

> > +cpu
> > +---
> > +
> > +Set the default CPU.
> > +
> > +Arguments:
> > +
> > +- "index": the CPU's index (json-int)
> > +
> > +Example:
> > +
> > +{ "execute": "cpu", "arguments": { "index": 0 } }
> > +
> > +Note: CPUs' indexes are obtained with the 'query-cpus' command.
> >    
> 
> The default cpu is local to the monitor?

 I guess so, someone has reported to me that QMP's and user Monitor's
behavior diverge. Ie, the default cpu set in QMP doesn't show up as
default in the user Monitor.

> Not sure, but it may have been better to omit it and instead require an 
> explicit cpu argument in all commands that rely on it.  As it is, 
> multithreaded management programs will need to lock the monitor when 
> reading cpu registers, etc.
> 
> Or, we can add an optional cpu argument to all commands that depend on it.

 I'll have to think about it, didn't have the time to get into this though.

> > +
> > +migrate
> > +-------
> > +
> > +Migrate to URI.
> > +
> > +Arguments:
> > +
> > +- "blk": block migration, full disk copy (json-bool, optional)
> > +- "inc": incremental disk copy (json-bool, optional)
> >    
> 
> Don't these need to be per-disk?

 You mean the documentation should say that or should we be able to
specify the disk some way?

> > +- "uri": Destination URI (json-string)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> > +
> > +Notes:
> > +
> > +(1) The 'query-migrate' command should be used to check migration's progress
> > +    and final result (this information is provided by the 'status' member)
> >    
> 
> Is there an event the provides completion notification?  I don't see one 
> in qemu-events.txt, but I think there should be one.

 Juan is working on it.

> > +(2) All boolean arguments default to false
> > +(3) The user Monitor's "detach" argument is invalid in QMP and should not
> > +    be used
> > +
> > +
> > +migrate_set_downtime
> > +--------------------
> > +
> > +Set maximum tolerated downtime (in seconds) for migrations.
> > +
> > +Arguments:
> > +
> > +- "value": maximum downtime (json-number)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate_set_downtime", "arguments": { "value": 60 } }
> >    
> 
> The example doesn't match reality well, suggest 0.1.
> 
> Would have been nicer as migrate_set_parameters downtime: 0.1, we can do 
> that when we add more parameters.
> 
> > +
> > +migrate_set_speed
> > +-----------------
> > +
> > +Set maximum speed for migrations.
> > +
> > +Arguments:
> > +
> > +- "value": maximum speed (json-number)
> > +
> > +Example:
> > +
> > +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
> >    
> 
> Oh, we do have more.
> 
> Please document the units for this value (bits per second)?
> 
> > +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.

> > +query-commands
> > +--------------
> > +
> > +List QMP available commands.
> > +
> > +Each command is represented by a json-object, the returned value is a json-array
> > +of all commands.
> > +
> > +Each json-object contain:
> > +
> > +- "name": command's name (json-string)
> > +
> > +Example:
> > +
> > +{
> > +   "return":[
> > +      {
> > +         "name":"query-balloon"
> > +      },
> > +      {
> > +         "name":"system_powerdown"
> > +      }
> > +   ]
> > +}
> > +
> > +Note: This example has been shortened as the real response is too long.
> >    
> 
> Presumably, this will be extended with a description of the arguments 
> and return types?

 Yes, we'll have one for events as well.

 This is self-description, our next big feature.

> > +query-cpus
> > +----------
> > +
> > +Show CPU information.
> > +
> > +Return a json-array. Each CPU is represented by a json-object, which contains:
> > +
> > +- "cpu": CPU index (json-int)
> > +- "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)
> >    
> 
> That's a bit sad.  It makes it difficult to use with stub generators.
> 
> Can we force everything to "pc"?  It will break non-x86, but there 
> aren't likely to be many QMP users using for those archs at present.

 Ok, Markus seems to think the same so I'll change it, a "arch" key
can be added later if needed.

> > +query-mice
> > +----------
> > +
> > +Show VM mice information.
> > +
> > +Each mouse is represented by a json-object, the returned value is a json-array
> > +of all mice.
> > +
> > +The mouse json-object contains the following:
> > +
> > +- "name": mouse's name (json-string)
> > +- "index": mouse's index (json-int)
> > +- "current": true if this mouse is receiving events, false otherwise (json-bool)
> >    
> 
> What does this mean?

 It's the mouse that will move, I guess.

> > +- "absolute": true if the mouse generates absolute input events (json-bool)
> > +
> >
> > +
> > +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)
> >    
> 
> Please list all possible values here (what's the status before the first 
> migration is started?

 The command returns "void", Markus has some suggestions on how to fix that.

> Very nice overall.  It is *way* easier to review documentation than 
> piles of code, so I suggest starting with documentation from now on.  
> I've adopted this approach in kvm.

 Absolutely, will work on v3 and resubmit.

On Thu, May 13, 2010 at 11:55:24AM -0300, Luiz Capitulino wrote:
> On Thu, 13 May 2010 16:48:13 +0300
> Avi Kivity <avi@redhat.com> wrote:
> 
> 
> > > +change
> > > +------
> > > +
> > > +Change a removable medium or VNC configuration.
> > >    
> > 
> > This is sad.  Would anyone write a C function with a similar description?
> 
>  I wouldn't, but someone did for qemu.
> 
> > Do we have this in 0.12?  If not we can change (...) it.  If we do, it's 
> > a more difficult deprecation process.
> 
>  Yes, we do and it's used by libvirt iirc.

This command has been in QEMU for quite a long time now (0.9.x IIRC).

IIRC, Gerd's SPICE proposal should include a brand new QMP command for
setting both VNC and SPICE authentication information, since it has
more than just a single password arg. At that point this use of change
for VNC could be deprecated

Regards,
Daniel

On 05/13/2010 05:55 PM, Luiz Capitulino wrote:
>
>
>>> +change
>>> +------
>>> +
>>> +Change a removable medium or VNC configuration.
>>>
>>>        
>> This is sad.  Would anyone write a C function with a similar description?
>>      
>   I wouldn't, but someone did for qemu.
>
>    
>> Do we have this in 0.12?  If not we can change (...) it.  If we do, it's
>> a more difficult deprecation process.
>>      
>   Yes, we do and it's used by libvirt iirc.
>    

I see.  Perhaps we can have change-storage and change-vnc, and deprecate 
the multiplexer.

>>> +cpu
>>> +---
>>> +
>>> +Set the default CPU.
>>> +
>>> +Arguments:
>>> +
>>> +- "index": the CPU's index (json-int)
>>> +
>>> +Example:
>>> +
>>> +{ "execute": "cpu", "arguments": { "index": 0 } }
>>> +
>>> +Note: CPUs' indexes are obtained with the 'query-cpus' command.
>>>
>>>        
>> The default cpu is local to the monitor?
>>      
>   I guess so, someone has reported to me that QMP's and user Monitor's
> behavior diverge. Ie, the default cpu set in QMP doesn't show up as
> default in the user Monitor.
>    

That's because it's local: each monitor has its own default cpu:
>
> struct Monitor {
>     ...
>     CPUState *mon_cpu;

This needs to be documented.


>>> +
>>> +migrate
>>> +-------
>>> +
>>> +Migrate to URI.
>>> +
>>> +Arguments:
>>> +
>>> +- "blk": block migration, full disk copy (json-bool, optional)
>>> +- "inc": incremental disk copy (json-bool, optional)
>>>
>>>        
>> Don't these need to be per-disk?
>>      
>   You mean the documentation should say that or should we be able to
> specify the disk some way?
>    

Both (with the block-migrate implementation leading the way).  It's 
reasonable to have one disk on shared storage and another on local 
storage.  For example, your swap device could be local while the main 
disk resides on a SAN/NAS.

>>> +--------
>>> +
>>> +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.
>    

That's what review is for, and since it's harder to review code, we only 
caught this now.

>    
>>> +query-mice
>>> +----------
>>> +
>>> +Show VM mice information.
>>> +
>>> +Each mouse is represented by a json-object, the returned value is a json-array
>>> +of all mice.
>>> +
>>> +The mouse json-object contains the following:
>>> +
>>> +- "name": mouse's name (json-string)
>>> +- "index": mouse's index (json-int)
>>> +- "current": true if this mouse is receiving events, false otherwise (json-bool)
>>>
>>>        
>> What does this mean?
>>      
>   It's the mouse that will move, I guess.
>    

Perhaps the mouse that is connected to vnc or SDL.  Eventually we'll 
need to support multiple host mice, though perhaps not in the next few 
years.

On 05/13/2010 06:01 PM, Daniel P. Berrange wrote:
>
>>   Yes, we do and it's used by libvirt iirc.
>>      
> This command has been in QEMU for quite a long time now (0.9.x IIRC).
>    

It wasn't in QMP until 0.12.  We shouldn't have put it there in that form.

> IIRC, Gerd's SPICE proposal should include a brand new QMP command for
> setting both VNC and SPICE authentication information, since it has
> more than just a single password arg. At that point this use of change
> for VNC could be deprecated
>    

Yes.  Yet management tools who want to support 0.12 will need to support 
the awkward multiplexor as well.

On Thu, 13 May 2010 19:23:11 +0300
Avi Kivity <avi@redhat.com> wrote:

> On 05/13/2010 06:01 PM, Daniel P. Berrange wrote:
> >
> >>   Yes, we do and it's used by libvirt iirc.
> >>      
> > This command has been in QEMU for quite a long time now (0.9.x IIRC).
> >    
> 
> It wasn't in QMP until 0.12.  We shouldn't have put it there in that form.

 Your main concern is stability? QMP in 0.12 and current are in preview
mode, ie. no stability contract until 0.13.

 We broke it already with a few changes (like capabilities support), so
anyone using QMP in 0.12 will have to update, although I don't think QMP
in 0.12 is that useful.

 Having said that, it's been stable for several weeks now and incompatible
changes have to be coordinated with libvirt.

 So, if you think this is worth the trouble someone could do it.

PS: I remember Anthony saying that he'd put somewhere that QMP is a
    preview version in 0.12. But I did not find anything, will submit
    a patch for stable.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 13 May 2010 19:23:11 +0300
> Avi Kivity <avi@redhat.com> wrote:
>
>> On 05/13/2010 06:01 PM, Daniel P. Berrange wrote:
>> >
>> >>   Yes, we do and it's used by libvirt iirc.
>> >>      
>> > This command has been in QEMU for quite a long time now (0.9.x IIRC).
>> >    
>> 
>> It wasn't in QMP until 0.12.  We shouldn't have put it there in that form.
>
>  Your main concern is stability? QMP in 0.12 and current are in preview
> mode, ie. no stability contract until 0.13.

And we absolutely need to make use of that license to improve stuff.
Premature stability leads to stable crap.  Having to get everything
right the first time is just too hard for mortals.

>  We broke it already with a few changes (like capabilities support), so
> anyone using QMP in 0.12 will have to update, although I don't think QMP
> in 0.12 is that useful.
>
>  Having said that, it's been stable for several weeks now and incompatible
> changes have to be coordinated with libvirt.

Yes, we should do our best not to break libvirt, and to help them out
when we have to break it.  That said, QMP is our chance to create a
decent management interface.  We really shouldn't drag in crap from the
human monitor carelessly.

>  So, if you think this is worth the trouble someone could do it.
>
> PS: I remember Anthony saying that he'd put somewhere that QMP is a
>     preview version in 0.12. But I did not find anything, will submit
>     a patch for stable.

I believe 0.12's QMP is too incomplete for serious use, so the risk of
somebody investing big into 0.12's QMP only to get blindsided by 0.13's
should be really low.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 13 May 2010 16:48:13 +0300
> Avi Kivity <avi@redhat.com> 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.

[...]

Avi Kivity <avi@redhat.com> writes:

> 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.
[...]
>> +migrate_set_downtime
>> +--------------------
>> +
>> +Set maximum tolerated downtime (in seconds) for migrations.
>> +
>> +Arguments:
>> +
>> +- "value": maximum downtime (json-number)
>> +
>> +Example:
>> +
>> +{ "execute": "migrate_set_downtime", "arguments": { "value": 60 } }
>>    
>
> The example doesn't match reality well, suggest 0.1.
>
> Would have been nicer as migrate_set_parameters downtime: 0.1, we can
> do that when we add more parameters.

I like the idea.

>> +
>> +migrate_set_speed
>> +-----------------
>> +
>> +Set maximum speed for migrations.
>> +
>> +Arguments:
>> +
>> +- "value": maximum speed (json-number)
>> +
>> +Example:
>> +
>> +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>>    
>
> Oh, we do have more.
>
> Please document the units for this value (bits per second)?

bytes per second?

>> +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"?
>
>> +- "size": the memory size (json-int)
>>    
>
> In bytes.

All sizes are in bytes.  Any exceptions are bugs.

That said, it's okay for reference documentation to repeat such things
over and over.

[...]
>> +query-pci
>> +---------
>> +
>> +PCI buses and devices information.
>> +
>> +The returned value is a json-array of all buses. Each bus is represented by
>> +a json-object, which has a key with a json-array of all PCI devices attached
>> +to it. Each device is represented by a json-object.
>> +
>> +The bus json-object contains the following:
>> +
>> +- "bus": bus number (json-int)
>> +- "devices": a json-array of json-objects, each json-object represents a
>> +             PCI device
>> +
>> +The PCI device json-object contains the following:
>> +
>> +- "bus": identical to the parent's bus number (json-int)
>> +- "slot": slot number (json-int)
>> +- "function": function number (json-int)
>>    
>
> Would have been nicer as a nested object (list of buses, each
> containing a list of slots, each containing a list of functions).

We have a list of buses, each containing a list of device functions.
Not sure the additional level of nesting you propose buys us anything.

I figure we can still change this if we want.  libvirt uses info pci
only with pre-0.12 QEMU, over the human monitor.

>> +- "class_info": a json-object containing:
>> +     - "desc": device class description (json-string, optional)
>> +     - "class": device class number (json-int)
>> +- "id": a json-object containing:
>> +     - "device": device ID (json-int)
>> +     - "vendor": vendor ID (json-int)
>> +- "irq": device's IRQ if assigned (json-int, optional)
>>    
>
> Please note here that the OS may ignore it, so may not be authoritative.
>
>> +- "qdev_id": qdev id string (json-string)
>> +- "pci_bridge": It's a json-object, only present if this device is a
>> +                PCI bridge, contains:
>> +     - "bus": bus number (json-int)
>> +     - "secondary": secondary bus number (json-int)
>> +     - "subordinate": subordinate bus number (json-int)
>> +     - "io_range": a json-object with memory range information (json-int)
>>    
>
> Object or int?

Delete "(json-int)", add suitable "see below".

>> +     - "memory_range": a json-object with memory range information (json-int)
>>    
> Object or int?

Delete "(json-int)", add suitable "see below".

>> +     - "prefetchable_range": a json-object with memory range
>> +                             information (json-int)
>>    
>
> Format not understood.

Delete "(json-int)", add suitable "see below".

>> +     - "devices": a json-array of PCI devices if there's any attached (optional)
>>    
>
> What is the format of an array element?

Yes, that's not clear here.

[...]

Avi, thanks a lot for reviewing this!

Am 12.05.2010 23:17, schrieb Luiz Capitulino:
> On Wed, 12 May 2010 18:48:38 +0200
> Markus Armbruster <armbru@redhat.com> 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?
> 
>>> +- "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"

Looks right. But you probably don't want to mention those drivers twice
that are present in both raw-posix.c and raw-win32.c.

Kevin

On Fri, 14 May 2010 10:39:29 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 13 May 2010 16:48:13 +0300
> > Avi Kivity <avi@redhat.com> 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..

On 05/14/2010 11:50 AM, Markus Armbruster wrote:
>
>>> +
>>> +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>>>
>>>        
>> Oh, we do have more.
>>
>> Please document the units for this value (bits per second)?
>>      
> bytes per second?
>    

Bandwidth is typically measured in bits per second.

>>> +- "bus": identical to the parent's bus number (json-int)
>>> +- "slot": slot number (json-int)
>>> +- "function": function number (json-int)
>>>
>>>        
>> Would have been nicer as a nested object (list of buses, each
>> containing a list of slots, each containing a list of functions).
>>      
> We have a list of buses, each containing a list of device functions.
> Not sure the additional level of nesting you propose buys us anything.
>    

A slot is the hotpluggable entity.  Open your computer and you can 
actually see them.

btw would be good to list empty slots as well.

What about PCI domains?

On 05/14/2010 11:33 AM, Markus Armbruster wrote:
>
> And we absolutely need to make use of that license to improve stuff.
> Premature stability leads to stable crap.  Having to get everything
> right the first time is just too hard for mortals.
>    

If that is the way we go (and I'm not sure what's the right thing here) 
then I suggest the following strategy:

- fix the patch so it describes the current state of things accurately
- merge
- post patches that change both the code and the documentation to fix 
protocol problems

That is, separate bugs in the documentation from bugs in the protocol; 
that allows new commands to be implemented and documented in parallel 
with fixing the old commands.

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.

Even if it's temporary - maintaining it in a separate file looks rather
unhandy.

Can't we generate the per-command documentation snippets also from
qemu-monitor.hx and merge them with a header/footer into some text file?
That .hx file is the one anyone adding/converting commands has to touch
anyway.

Jan

On 05/14/2010 07:52 PM, Jan Kiszka wrote:
>
>> 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.
>>      
> Even if it's temporary - maintaining it in a separate file looks rather
> unhandy.
>
> Can't we generate the per-command documentation snippets also from
> qemu-monitor.hx and merge them with a header/footer into some text file?
> That .hx file is the one anyone adding/converting commands has to touch
> anyway.
>    

If we do, then the generated documentation should be included in the 
patch changelog for review.

On 05/14/2010 08:01 PM, Avi Kivity wrote:
> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
>>
>>> 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.
>> Even if it's temporary - maintaining it in a separate file looks rather
>> unhandy.
>>
>> Can't we generate the per-command documentation snippets also from
>> qemu-monitor.hx and merge them with a header/footer into some text file?
>> That .hx file is the one anyone adding/converting commands has to touch
>> anyway.
>
> If we do, then the generated documentation should be included in the 
> patch changelog for review.
>

I mean, a patch introducing or modifying a monitor command.

Avi Kivity <avi@redhat.com> writes:

> On 05/14/2010 11:50 AM, Markus Armbruster wrote:
>>
>>>> +
>>>> +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>>>>
>>>>        
>>> Oh, we do have more.
>>>
>>> Please document the units for this value (bits per second)?
>>>      
>> bytes per second?
>>    
>
> Bandwidth is typically measured in bits per second.

The question is what unit the code actually uses.  What it should use is
separate, if valid question.

>>>> +- "bus": identical to the parent's bus number (json-int)
>>>> +- "slot": slot number (json-int)
>>>> +- "function": function number (json-int)
>>>>
>>>>        
>>> Would have been nicer as a nested object (list of buses, each
>>> containing a list of slots, each containing a list of functions).
>>>      
>> We have a list of buses, each containing a list of device functions.
>> Not sure the additional level of nesting you propose buys us anything.
>>    
>
> A slot is the hotpluggable entity.  Open your computer and you can
> actually see them.

QEMU doesn't really know that.

> btw would be good to list empty slots as well.

Why?

> What about PCI domains?

Good point.  Better to provide for them neatly now, instead of kludging
them in later.

Avi Kivity <avi@redhat.com> writes:

> On 05/14/2010 11:33 AM, Markus Armbruster wrote:
>>
>> And we absolutely need to make use of that license to improve stuff.
>> Premature stability leads to stable crap.  Having to get everything
>> right the first time is just too hard for mortals.
>>    
>
> If that is the way we go (and I'm not sure what's the right thing
> here) then I suggest the following strategy:
>
> - fix the patch so it describes the current state of things accurately
> - merge
> - post patches that change both the code and the documentation to fix
> protocol problems
>
> That is, separate bugs in the documentation from bugs in the protocol;
> that allows new commands to be implemented and documented in parallel
> with fixing the old commands.

Step 1: Move the existing documentation out of the source files
verbatim.

Step 2: Fix it up so that it describes the current state of things
accurately.

Step 3+: Fix the protocol problems.

Avi Kivity wrote:
> On 05/14/2010 08:01 PM, Avi Kivity wrote:
>> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
>>>> 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.
>>> Even if it's temporary - maintaining it in a separate file looks rather
>>> unhandy.
>>>
>>> Can't we generate the per-command documentation snippets also from
>>> qemu-monitor.hx and merge them with a header/footer into some text file?
>>> That .hx file is the one anyone adding/converting commands has to touch
>>> anyway.
>> If we do, then the generated documentation should be included in the 
>> patch changelog for review.
>>
> 
> I mean, a patch introducing or modifying a monitor command.

The snippets should be readable by themselves. I'm only proposing to
keep them in the central file, at the same location where the others
are. There is no difference compared to existing monitor commands, we
just add the third documentation snippet, this time for QMP.

Distributing a generated QMP/qmp-commands.txt is another thing, maybe a
useful one.

Jan

On 05/14/2010 08:03 PM, Markus Armbruster wrote:
> Avi Kivity<avi@redhat.com>  writes:
>
>    
>> On 05/14/2010 11:50 AM, Markus Armbruster wrote:
>>      
>>>        
>>>>> +
>>>>> +{ "execute": "migrate_set_speed", "arguments": { "value": 1024 } }
>>>>>
>>>>>
>>>>>            
>>>> Oh, we do have more.
>>>>
>>>> Please document the units for this value (bits per second)?
>>>>
>>>>          
>>> bytes per second?
>>>
>>>        
>> Bandwidth is typically measured in bits per second.
>>      
> The question is what unit the code actually uses.  What it should use is
> separate, if valid question.
>    

Right.

>>> We have a list of buses, each containing a list of device functions.
>>> Not sure the additional level of nesting you propose buys us anything.
>>>
>>>        
>> A slot is the hotpluggable entity.  Open your computer and you can
>> actually see them.
>>      
> QEMU doesn't really know that.
>    

How can that be?  Do we signal hotplug notifications to a function or to 
a slot?

Can we hotplug a single function in an already occupied slot?

>> btw would be good to list empty slots as well.
>>      
> Why?
>    

So management knows how many slots are available.

It's the difference between:

    Error: the device could not be hotplugged

and

<button disabled="yes">Hotplug device</button>

   or

   3 PCI slots free.

On 05/14/2010 08:08 PM, Jan Kiszka wrote:
>
>> I mean, a patch introducing or modifying a monitor command.
>>      
> The snippets should be readable by themselves.

They may be readable, but that doesn't mean anyone will read them.  It's 
a lot easier for someone who isn't involved in the code (the libvirt 
people, or me) to review standalone documentation than something 
embedded in a patch (typically at the very end).

> I'm only proposing to
> keep them in the central file, at the same location where the others
> are. There is no difference compared to existing monitor commands, we
> just add the third documentation snippet, this time for QMP.
>    

That's fine.  But the fact that a lot of review comments pointing out 
issues surface only now show that we need to make things easier for 
reviewers.  It's much harder to fix an external interface than an 
internal one.

> Distributing a generated QMP/qmp-commands.txt is another thing, maybe a
> useful one.
>    

Sure, like the other qemu documentation.

On Fri, 14 May 2010 19:03:36 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> > What about PCI domains?
> 
> Good point.  Better to provide for them neatly now, instead of kludging
> them in later.

 When I did this conversion I asked Micheal for help with that and he said
QEMU doesn't support PCI domains.

On Fri, 14 May 2010 19:08:07 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> Avi Kivity wrote:
> > On 05/14/2010 08:01 PM, Avi Kivity wrote:
> >> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
> >>>> 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.
> >>> Even if it's temporary - maintaining it in a separate file looks rather
> >>> unhandy.
> >>>
> >>> Can't we generate the per-command documentation snippets also from
> >>> qemu-monitor.hx and merge them with a header/footer into some text file?
> >>> That .hx file is the one anyone adding/converting commands has to touch
> >>> anyway.
> >> If we do, then the generated documentation should be included in the 
> >> patch changelog for review.
> >>
> > 
> > I mean, a patch introducing or modifying a monitor command.
> 
> The snippets should be readable by themselves. I'm only proposing to
> keep them in the central file, at the same location where the others
> are. There is no difference compared to existing monitor commands, we
> just add the third documentation snippet, this time for QMP.

 It's not only the snippets. We add a json type for each parameter, a
more descriptive info and notes. Only QMP commands should be shown
this way.

 I'm sure there's a way to hack the qemu's doc script to do all this,
but the right solution is to move _everything_ to json and generate good,
well formatted documentation from there (along with self-description).

 Also, adapting things will take time and this will delay even more this
doc to be merged, which is what I'm trying to avoid.

On 05/15/2010 01:54 AM, Luiz Capitulino wrote:
> On Fri, 14 May 2010 19:03:36 +0200
> Markus Armbruster<armbru@redhat.com>  wrote:
>
>    
>>> What about PCI domains?
>>>        
>> Good point.  Better to provide for them neatly now, instead of kludging
>> them in later.
>>      
>   When I did this conversion I asked Micheal for help with that and he said
> QEMU doesn't support PCI domains.
>    

That's very different from "will never support pci domains".

The protocol must be forward looking, or we will need endless fixes for it.

Luiz Capitulino wrote:
> On Fri, 14 May 2010 19:08:07 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> Avi Kivity wrote:
>>> On 05/14/2010 08:01 PM, Avi Kivity wrote:
>>>> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
>>>>>> 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.
>>>>> Even if it's temporary - maintaining it in a separate file looks rather
>>>>> unhandy.
>>>>>
>>>>> Can't we generate the per-command documentation snippets also from
>>>>> qemu-monitor.hx and merge them with a header/footer into some text file?
>>>>> That .hx file is the one anyone adding/converting commands has to touch
>>>>> anyway.
>>>> If we do, then the generated documentation should be included in the 
>>>> patch changelog for review.
>>>>
>>> I mean, a patch introducing or modifying a monitor command.
>> The snippets should be readable by themselves. I'm only proposing to
>> keep them in the central file, at the same location where the others
>> are. There is no difference compared to existing monitor commands, we
>> just add the third documentation snippet, this time for QMP.
> 
>  It's not only the snippets. We add a json type for each parameter, a
> more descriptive info and notes. Only QMP commands should be shown
> this way.

Whatever its semantic is, technically it's a text block of a few lines
that has to be added somewhere.

> 
>  I'm sure there's a way to hack the qemu's doc script to do all this,
> but the right solution is to move _everything_ to json and generate good,
> well formatted documentation from there (along with self-description).

I'm not talking about The Right solution, I'm talking about a more handy
temporary setup. When do you plan to refactor the command documentation
that way? And how many commands will be touched in the meantime?

> 
>  Also, adapting things will take time and this will delay even more this
> doc to be merged, which is what I'm trying to avoid.
> 

I can provide you the patch for hxtool and Makefile (a few lines), and
I'm willing to split up the existing doc, just drop me a note. So
nothing needs to be delayed any further.

Jan

Avi Kivity <avi@redhat.com> writes:

> On 05/14/2010 08:03 PM, Markus Armbruster wrote:
>> Avi Kivity<avi@redhat.com>  writes:
>>
>>    
>>> On 05/14/2010 11:50 AM, Markus Armbruster wrote:
[...]
>>>> We have a list of buses, each containing a list of device functions.
>>>> Not sure the additional level of nesting you propose buys us anything.
>>>>
>>>>        
>>> A slot is the hotpluggable entity.  Open your computer and you can
>>> actually see them.
>>>      
>> QEMU doesn't really know that.
>>    
>
> How can that be?  Do we signal hotplug notifications to a function or
> to a slot?
>
> Can we hotplug a single function in an already occupied slot?

What I meant to say: we have no concept of "slot" in the higher level
interfaces, we have only bus and device.

If a PCI device has multiple functions, we have a separate qdev device
for each function.  You can't unplug a "slot" (concept doesn't exist in
qdev), only a qdev device.  Naturally, when you unplug a qdev device,
all functions in the same PCI slot need to go.  This happens deep down
in the bowels of ACPI, in piix4_device_hotplug().  qdev is not aware of
this magic relation between the qdev devices for functions in the same
slot.

[...]

Avi Kivity <avi@redhat.com> writes:

> On 05/15/2010 01:54 AM, Luiz Capitulino wrote:
>> On Fri, 14 May 2010 19:03:36 +0200
>> Markus Armbruster<armbru@redhat.com>  wrote:
>>
>>    
>>>> What about PCI domains?
>>>>        
>>> Good point.  Better to provide for them neatly now, instead of kludging
>>> them in later.
>>>      
>>   When I did this conversion I asked Micheal for help with that and he said
>> QEMU doesn't support PCI domains.
>>    
>
> That's very different from "will never support pci domains".
>
> The protocol must be forward looking, or we will need endless fixes for it.

Precisely.

On 05/17/2010 11:27 AM, Markus Armbruster wrote:
>
>    
>>>> A slot is the hotpluggable entity.  Open your computer and you can
>>>> actually see them.
>>>>
>>>>          
>>> QEMU doesn't really know that.
>>>
>>>        
>> How can that be?  Do we signal hotplug notifications to a function or
>> to a slot?
>>
>> Can we hotplug a single function in an already occupied slot?
>>      
> What I meant to say: we have no concept of "slot" in the higher level
> interfaces, we have only bus and device.
>
> If a PCI device has multiple functions, we have a separate qdev device
> for each function.  You can't unplug a "slot" (concept doesn't exist in
> qdev), only a qdev device.  Naturally, when you unplug a qdev device,
> all functions in the same PCI slot need to go.  This happens deep down
> in the bowels of ACPI, in piix4_device_hotplug().  qdev is not aware of
> this magic relation between the qdev devices for functions in the same
> slot.
>    

IMO, that's a serious bug.  A slot is a user visible entity, both in 
that devices can only be hotplugged only as slots, not functions, and to 
determine the maximum number of devices you can add.  If the user knows 
about it, qemu should too.

We can easily represent a slot/device as a qbus with each of the 
functions as devices attached to it.

Avi Kivity <avi@redhat.com> writes:

> On 05/17/2010 11:27 AM, Markus Armbruster wrote:
>>
>>    
>>>>> A slot is the hotpluggable entity.  Open your computer and you can
>>>>> actually see them.
>>>>>
>>>>>          
>>>> QEMU doesn't really know that.
>>>>
>>>>        
>>> How can that be?  Do we signal hotplug notifications to a function or
>>> to a slot?
>>>
>>> Can we hotplug a single function in an already occupied slot?
>>>      
>> What I meant to say: we have no concept of "slot" in the higher level
>> interfaces, we have only bus and device.
>>
>> If a PCI device has multiple functions, we have a separate qdev device
>> for each function.  You can't unplug a "slot" (concept doesn't exist in
>> qdev), only a qdev device.  Naturally, when you unplug a qdev device,
>> all functions in the same PCI slot need to go.  This happens deep down
>> in the bowels of ACPI, in piix4_device_hotplug().  qdev is not aware of
>> this magic relation between the qdev devices for functions in the same
>> slot.
>>    
>
> IMO, that's a serious bug.  A slot is a user visible entity, both in
> that devices can only be hotplugged only as slots, not functions, and
> to determine the maximum number of devices you can add.  If the user
> knows about it, qemu should too.
>
> We can easily represent a slot/device as a qbus with each of the
> functions as devices attached to it.

Dunno.  Gerd, what do you think?

On Sat, 15 May 2010 10:42:44 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Luiz Capitulino wrote:
> > On Fri, 14 May 2010 19:08:07 +0200
> > Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > 
> >> Avi Kivity wrote:
> >>> On 05/14/2010 08:01 PM, Avi Kivity wrote:
> >>>> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
> >>>>>> 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.
> >>>>> Even if it's temporary - maintaining it in a separate file looks rather
> >>>>> unhandy.
> >>>>>
> >>>>> Can't we generate the per-command documentation snippets also from
> >>>>> qemu-monitor.hx and merge them with a header/footer into some text file?
> >>>>> That .hx file is the one anyone adding/converting commands has to touch
> >>>>> anyway.
> >>>> If we do, then the generated documentation should be included in the 
> >>>> patch changelog for review.
> >>>>
> >>> I mean, a patch introducing or modifying a monitor command.
> >> The snippets should be readable by themselves. I'm only proposing to
> >> keep them in the central file, at the same location where the others
> >> are. There is no difference compared to existing monitor commands, we
> >> just add the third documentation snippet, this time for QMP.
> > 
> >  It's not only the snippets. We add a json type for each parameter, a
> > more descriptive info and notes. Only QMP commands should be shown
> > this way.
> 
> Whatever its semantic is, technically it's a text block of a few lines
> that has to be added somewhere.
> 
> > 
> >  I'm sure there's a way to hack the qemu's doc script to do all this,
> > but the right solution is to move _everything_ to json and generate good,
> > well formatted documentation from there (along with self-description).
> 
> I'm not talking about The Right solution, I'm talking about a more handy
> temporary setup. When do you plan to refactor the command documentation
> that way? And how many commands will be touched in the meantime?

 It's something we would like to do ASAP, but there are a number of things
we'll have to do first: bug fixes and some commands libvirt wants to use.

> >  Also, adapting things will take time and this will delay even more this
> > doc to be merged, which is what I'm trying to avoid.
> > 
> 
> I can provide you the patch for hxtool and Makefile (a few lines), and
> I'm willing to split up the existing doc, just drop me a note. So
> nothing needs to be delayed any further.

 I'd love that.

On 05/17/2010 06:19 AM, Markus Armbruster wrote:
> Avi Kivity<avi@redhat.com>  writes:
>
>    
>> On 05/17/2010 11:27 AM, Markus Armbruster wrote:
>>      
>>>
>>>        
>>>>>> A slot is the hotpluggable entity.  Open your computer and you can
>>>>>> actually see them.
>>>>>>
>>>>>>
>>>>>>              
>>>>> QEMU doesn't really know that.
>>>>>
>>>>>
>>>>>            
>>>> How can that be?  Do we signal hotplug notifications to a function or
>>>> to a slot?
>>>>
>>>> Can we hotplug a single function in an already occupied slot?
>>>>
>>>>          
>>> What I meant to say: we have no concept of "slot" in the higher level
>>> interfaces, we have only bus and device.
>>>
>>> If a PCI device has multiple functions, we have a separate qdev device
>>> for each function.  You can't unplug a "slot" (concept doesn't exist in
>>> qdev), only a qdev device.  Naturally, when you unplug a qdev device,
>>> all functions in the same PCI slot need to go.  This happens deep down
>>> in the bowels of ACPI, in piix4_device_hotplug().  qdev is not aware of
>>> this magic relation between the qdev devices for functions in the same
>>> slot.
>>>
>>>        
>> IMO, that's a serious bug.  A slot is a user visible entity, both in
>> that devices can only be hotplugged only as slots, not functions, and
>> to determine the maximum number of devices you can add.  If the user
>> knows about it, qemu should too.
>>
>> We can easily represent a slot/device as a qbus with each of the
>> functions as devices attached to it.
>>      
> Dunno.  Gerd, what do you think?
>    

Personally, I would think slots should be a property of the PCI bus (ala 
qdev).  It's something that you probably want to be configurable since 
it's not very common to see machines with 32 slots.

That said, since it affects the ACPI tables, it may be difficult to do 
in practice.

Regards,

Anthony Liguori

On 05/15/2010 01:19 AM, Avi Kivity wrote:
> On 05/15/2010 01:54 AM, Luiz Capitulino wrote:
>> On Fri, 14 May 2010 19:03:36 +0200
>> Markus Armbruster<armbru@redhat.com>  wrote:
>>
>>>> What about PCI domains?
>>> Good point.  Better to provide for them neatly now, instead of kludging
>>> them in later.
>>   When I did this conversion I asked Micheal for help with that and 
>> he said
>> QEMU doesn't support PCI domains.
>
> That's very different from "will never support pci domains".
>
> The protocol must be forward looking, or we will need endless fixes 
> for it.

But we can always add a domain property to extend the address (with a 
default domain of 0).

Regards,

Anthony Liguori

On 05/17/2010 09:10 PM, Anthony Liguori wrote:
>>
>> The protocol must be forward looking, or we will need endless fixes 
>> for it.
>
>
> But we can always add a domain property to extend the address (with a 
> default domain of 0).
>

That's what I meant by endless fixes.  Each one is reasonable by itself, 
but if you do enough of them, people go blech when they see the 
documentation, or get it wrong if they don't.

Hi,

>>> IMO, that's a serious bug. A slot is a user visible entity, both in
>>> that devices can only be hotplugged only as slots, not functions, and
>>> to determine the maximum number of devices you can add. If the user
>>> knows about it, qemu should too.
>>>
>>> We can easily represent a slot/device as a qbus with each of the
>>> functions as devices attached to it.

Point being?

>> Dunno. Gerd, what do you think?

There is a PCIDevice for each function.  PCIDevice->devfn (aka addr 
property) contains slot+function.  So hw/pci.c can figure which device 
functions belong to the same slot.  The pci hotplug code might need some 
fixes to handle multi-function devices correctly though (I guess this is 
the original issue?).  unplug is probably easy, plug might be harder. 
You have to plug-in all functions belonging to the slot first, then 
signal the guest that the slot has been hotplugged, which might need 
changes in the monitor protocol.

cheers,
   Gerd

On 05/17/2010 10:21 PM, Gerd Hoffmann wrote:
>
>>> Dunno. Gerd, what do you think?
>
> There is a PCIDevice for each function.  PCIDevice->devfn (aka addr 
> property) contains slot+function.  So hw/pci.c can figure which device 
> functions belong to the same slot.  The pci hotplug code might need 
> some fixes to handle multi-function devices correctly though (I guess 
> this is the original issue?).

The original issue is that the code does not model reality.  Yes, it 
only hurts with multifunction devices.

We need a PCIDevice->devaddr and PCIDevice->function[]->fnaddr.


> unplug is probably easy, plug might be harder. You have to plug-in all 
> functions belonging to the slot first, then signal the guest that the 
> slot has been hotplugged, which might need changes in the monitor 
> protocol.

We need to plug in a device, not functions.

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/15/2010 01:19 AM, Avi Kivity wrote:
>> On 05/15/2010 01:54 AM, Luiz Capitulino wrote:
>>> On Fri, 14 May 2010 19:03:36 +0200
>>> Markus Armbruster<armbru@redhat.com>  wrote:
>>>
>>>>> What about PCI domains?
>>>> Good point.  Better to provide for them neatly now, instead of kludging
>>>> them in later.
>>>   When I did this conversion I asked Micheal for help with that and
>>> he said
>>> QEMU doesn't support PCI domains.
>>
>> That's very different from "will never support pci domains".
>>
>> The protocol must be forward looking, or we will need endless fixes
>> for it.
>
> But we can always add a domain property to extend the address (with a
> default domain of 0).

Why not add it right away?

Note that if we'd decide to adopt Avi's suggestion to make this "a
nested object (list of buses, each containing a list of slots, each
containing a list of functions)", then we can't easily add domains
later, because that would insert a level of nesting near the top.

Jan Kiszka <jan.kiszka@web.de> writes:

> Luiz Capitulino wrote:
>> On Fri, 14 May 2010 19:08:07 +0200
>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> 
>>> Avi Kivity wrote:
>>>> On 05/14/2010 08:01 PM, Avi Kivity wrote:
>>>>> On 05/14/2010 07:52 PM, Jan Kiszka wrote:
>>>>>>> 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.
>>>>>> Even if it's temporary - maintaining it in a separate file looks rather
>>>>>> unhandy.
>>>>>>
>>>>>> Can't we generate the per-command documentation snippets also from
>>>>>> qemu-monitor.hx and merge them with a header/footer into some text file?
>>>>>> That .hx file is the one anyone adding/converting commands has to touch
>>>>>> anyway.
>>>>> If we do, then the generated documentation should be included in the 
>>>>> patch changelog for review.
>>>>>
>>>> I mean, a patch introducing or modifying a monitor command.
>>> The snippets should be readable by themselves. I'm only proposing to
>>> keep them in the central file, at the same location where the others
>>> are. There is no difference compared to existing monitor commands, we
>>> just add the third documentation snippet, this time for QMP.
>> 
>>  It's not only the snippets. We add a json type for each parameter, a
>> more descriptive info and notes. Only QMP commands should be shown
>> this way.
>
> Whatever its semantic is, technically it's a text block of a few lines
> that has to be added somewhere.

The current documentation is just a block of text, yes.  Fine for human
consumption.  Useless for QMP self-documentation, i.e. machine-readable
documentation exposed over QMP.  There, we need a bit more structure.

qemu-monitor.gx is a sub-optimal permanent place for command
documentation.  The command documentation source should live right next
to the command code, because that increases its chances to get updated
along with the code.

Naturally, we also need a way to generate something like
qmp-commands.txt from it, for easier review, and for the human users of
QMP.

>>  I'm sure there's a way to hack the qemu's doc script to do all this,
>> but the right solution is to move _everything_ to json and generate good,
>> well formatted documentation from there (along with self-description).
>
> I'm not talking about The Right solution, I'm talking about a more handy
> temporary setup. When do you plan to refactor the command documentation
> that way? And how many commands will be touched in the meantime?

We need self-documentation badly right after we declare QMP stable,
i.e. 0.13.  Because any change after that needs to be discoverable by
QMP clients.

Ideally, we put it in before 0.13.  If we can't pull that off, then
early in the 0.14 cycle.  Help appreciated!

>>  Also, adapting things will take time and this will delay even more this
>> doc to be merged, which is what I'm trying to avoid.
>> 
>
> I can provide you the patch for hxtool and Makefile (a few lines), and
> I'm willing to split up the existing doc, just drop me a note.

Moving it from one temporary, sub-optimal place (QMP/qmp-commands.txt)
to another temporary, sub-optimal place (qemu-monitor.hx) doesn't seem
like a win to me.  Should temporary macro wizardry be required for that
(hxtool patch), the deal gets even less appealing.

>                                                                So
> nothing needs to be delayed any further.

Well, it's being delayed :)

Let's commit the sucker as is.  We can still move it into
qemu-monitor.hx afterwards.  Commits are cheap, waiting for the perfect
patch isn't.

On Tue, 18 May 2010 11:51:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 05/15/2010 01:19 AM, Avi Kivity wrote:
> >> On 05/15/2010 01:54 AM, Luiz Capitulino wrote:
> >>> On Fri, 14 May 2010 19:03:36 +0200
> >>> Markus Armbruster<armbru@redhat.com>  wrote:
> >>>
> >>>>> What about PCI domains?
> >>>> Good point.  Better to provide for them neatly now, instead of kludging
> >>>> them in later.
> >>>   When I did this conversion I asked Micheal for help with that and
> >>> he said
> >>> QEMU doesn't support PCI domains.
> >>
> >> That's very different from "will never support pci domains".
> >>
> >> The protocol must be forward looking, or we will need endless fixes
> >> for it.
> >
> > But we can always add a domain property to extend the address (with a
> > default domain of 0).
> 
> Why not add it right away?

 The plan is to merge the doc and work on all changes suggested by
Avi (always coordinating with libvirt, of course).

 Although query-pci is not used there.

> Note that if we'd decide to adopt Avi's suggestion to make this "a
> nested object (list of buses, each containing a list of slots, each
> containing a list of functions)", then we can't easily add domains
> later, because that would insert a level of nesting near the top.

 Right.

On Tue, 18 May 2010 13:21:36 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Jan Kiszka <jan.kiszka@web.de> writes:

[...]

> > nothing needs to be delayed any further.
> 
> Well, it's being delayed :)
> 
> Let's commit the sucker as is.  We can still move it into
> qemu-monitor.hx afterwards.  Commits are cheap, waiting for the perfect
> patch isn't.

 Jan has already adapted qemu-monitor.hx and moved the entire doc into
there. I didn't take a look at the patches yet, though.

 Will do it today and work on the (_doc_) proposed changes in the process.