[Qemu-devel] [PATCH 0/2] QMP: Introduce query-qdm

[Qemu-devel] [PATCH 0/2] QMP: Introduce query-qdm

[Miguel Di Ciurcio Filho]

This series introduces the documentation for the query-qdm command and the
conversion of the monitor command 'info qdm' to QMP.

The documentation and code are based on a patch previously sent to qemu-devel
by Daniel P. Berrange:

http://lists.gnu.org/archive/html/qemu-devel/2010-06/msg00931.html

Changes from the Daniel's original patch:
- Split the patch in two, taking out the documentation from the code
- Reworded some parts of the documentation and added data types
- Small cleanups and renamed do_info_devices() to do_info_qdm()
- Added do_info_qdm_print() to be used in the monitor

Regards,

Miguel

[Qemu-devel] [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Miguel Di Ciurcio Filho]

---
 qemu-monitor.hx |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 9f62b94..5348899 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -2490,6 +2490,74 @@ STEXI
 show device tree
 @item info qdm
 show qdev device model list
+ETEXI
+SQMP
+query-qdm
+---------
+
+Describe the capabilities of all devices registered with qdev.
+
+The returned output is a list, each element is a json-object describing a single
+device type.
+
+Each json-object contains the following:
+
+- "name": the short name of the device (json-string)
+- "bus": the name of the bus type for the device (json-string)
+- "alias": an alias by which the device is also known (json-string, optional)
+- "description": a long description the device  (json-string, optional)
+- "creatable": whether this device can be created on command line (json-boolean)
+- "props": a list where each element is an json-object that describes a property
+of the device. Each json-object contains the following:
+     - "name": the short name of the property (json-string)
+     - "info": short description of the property (json-string)
+     - "type": the data type of the property value (json-string)
+
+Example:
+
+-> { "execute": "query-qdm" }
+<- {
+      "return": [
+        {
+           "name": "virtio-9p-pci",
+           "creatable": true,
+           "bus": "PCI",
+           "props": [
+             {
+                 "name": "indirect_desc",
+                 "type": "bit",
+                 "info": "on/off"
+             },
+             {
+                 "name": "mount_tag",
+                 "type": "string",
+                 "info": "string"
+             },
+             {
+                 "name": "fsdev",
+                 "type": "string",
+                 "info": "string"
+             }
+           ]
+        },
+        {
+           "name": "virtio-balloon-pci",
+           "creatable": true,
+           "bus": "PCI",
+           "props": [
+             {
+               "name": "indirect_desc",
+               "type": "bit",
+               "info": "on/off"
+             }
+           ]
+        },
+      ....
+    ]
+
+EQMP
+
+STEXI
 @item info roms
 show roms
 @end table
-- 
1.7.1

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Avi Kivity]

On 07/03/2010 12:27 AM, Miguel Di Ciurcio Filho wrote:
> ---
>   qemu-monitor.hx |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 68 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 9f62b94..5348899 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -2490,6 +2490,74 @@ STEXI
>   show device tree
>   @item info qdm
>   show qdev device model list
> +ETEXI
> +SQMP
> +query-qdm
> +---------
> +
> +Describe the capabilities of all devices registered with qdev.
>    

Why the name qdm?

'query-available-devices' instead?

Otherwise looks pretty nice.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Miguel Di Ciurcio Filho]

On Sun, Jul 4, 2010 at 2:14 AM, Avi Kivity <avi@redhat.com> wrote:
>>  show device tree
>>  @item info qdm
>>  show qdev device model list
>> +ETEXI
>> +SQMP
>> +query-qdm
>> +---------
>> +
>> +Describe the capabilities of all devices registered with qdev.
>>
>
> Why the name qdm?
>
> 'query-available-devices' instead?
>
> Otherwise looks pretty nice.

No special reason, but it is already a known command that provides
this information in the monitor.

Regards,

Miguel

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Luiz Capitulino]

On Fri,  2 Jul 2010 18:27:02 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> ---
>  qemu-monitor.hx |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> index 9f62b94..5348899 100644
> --- a/qemu-monitor.hx
> +++ b/qemu-monitor.hx
> @@ -2490,6 +2490,74 @@ STEXI
>  show device tree
>  @item info qdm
>  show qdev device model list
> +ETEXI
> +SQMP
> +query-qdm
> +---------
> +
> +Describe the capabilities of all devices registered with qdev.
> +
> +The returned output is a list, each element is a json-object describing a single
> +device type.

s/The returned output is a list/The returned value is a json-array

> +
> +Each json-object contains the following:
> +
> +- "name": the short name of the device (json-string)

Why short? Isn't it the name itself?

> +- "bus": the name of the bus type for the device (json-string)

Do we need a list o possible values?

> +- "alias": an alias by which the device is also known (json-string, optional)
> +- "description": a long description the device  (json-string, optional)
> +- "creatable": whether this device can be created on command line (json-boolean)
> +- "props": a list where each element is an json-object that describes a property
> +of the device. Each json-object contains the following:

Suggest using "properties" (vs. "props")

> +     - "name": the short name of the property (json-string)

Why short? Isn't it the name itself?

> +     - "info": short description of the property (json-string)

You sure it's a description of the property? It seems to describe how to
set it (related, but slightly different).

Also, most of the time it seems to be an exact copy of "type". I suggest
to make it optional and only show it when it differs from "type".

> +     - "type": the data type of the property value (json-string)

We need a list o possible values, with a small explanation of each one.
Do we need the equivalent in json too?

> +
> +Example:
> +
> +-> { "execute": "query-qdm" }
> +<- {
> +      "return": [
> +        {
> +           "name": "virtio-9p-pci",
> +           "creatable": true,
> +           "bus": "PCI",
> +           "props": [
> +             {
> +                 "name": "indirect_desc",
> +                 "type": "bit",
> +                 "info": "on/off"
> +             },
> +             {
> +                 "name": "mount_tag",
> +                 "type": "string",
> +                 "info": "string"
> +             },
> +             {
> +                 "name": "fsdev",
> +                 "type": "string",
> +                 "info": "string"
> +             }
> +           ]
> +        },
> +        {
> +           "name": "virtio-balloon-pci",
> +           "creatable": true,
> +           "bus": "PCI",
> +           "props": [
> +             {
> +               "name": "indirect_desc",
> +               "type": "bit",
> +               "info": "on/off"
> +             }
> +           ]
> +        },
> +      ....
> +    ]

Suggest a NOTE saying this the equivalent of command-line options -device ?
and -device devname,?

> +
> +EQMP
> +
> +STEXI
>  @item info roms
>  show roms
>  @end table

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Miguel Di Ciurcio Filho]

On Mon, Jul 5, 2010 at 12:22 PM, Luiz Capitulino <lcapitulino@redhat.com> wrote:
>> +
>> +Describe the capabilities of all devices registered with qdev.
>> +
>> +The returned output is a list, each element is a json-object describing a single
>> +device type.
>
> s/The returned output is a list/The returned value is a json-array
>

Ack.

>> +
>> +Each json-object contains the following:
>> +
>> +- "name": the short name of the device (json-string)
>
> Why short? Isn't it the name itself?

Yeah, it is just the a name. I think the initial use for "short" from
Daniel was to distinguish from "name" and "description".

>
>> +- "bus": the name of the bus type for the device (json-string)
>
> Do we need a list o possible values?
>

I didn't find a central location for all the possible values. Although
running `egrep -C1 "static struct BusInfo" *.c` shows most names, I
hope. Does anyone more experienced could confirm that this is how I
can find out all bus types?

>> +- "alias": an alias by which the device is also known (json-string, optional)
>> +- "description": a long description the device  (json-string, optional)
>> +- "creatable": whether this device can be created on command line (json-boolean)
>> +- "props": a list where each element is an json-object that describes a property
>> +of the device. Each json-object contains the following:
>
> Suggest using "properties" (vs. "props")

Better indeed, ack.

>
>> +     - "name": the short name of the property (json-string)
>
> Why short? Isn't it the name itself?
>

No need for short here, IMHO. Ack.

>> +     - "info": short description of the property (json-string)
>
> You sure it's a description of the property? It seems to describe how to
> set it (related, but slightly different).
>
> Also, most of the time it seems to be an exact copy of "type". I suggest
> to make it optional and only show it when it differs from "type".
>
>> +     - "type": the data type of the property value (json-string)
>

Looking deeper into it I think it is a bit clear now.

DeviceInfo
    |__Property
    |       | char name
    |       | PropertyInfo info
    |               |  char name
    |               |  enum PropertyType type
    |__Property
            | char name
            | PropertyInfo info
                    |  char name
                    |  enum PropertyType type

So, for something like this:

"properties": [
             {
                 "name": "indirect_desc",
                 "type": "bit",
                 "info": "on/off"
             },

"name" is Property->name
"type" is Property->info->type
"info" is Property->info->name

"name" and "type" are relevant, but I think "info" is not.

e.g.:
$ qemu -device e1000,?
e1000.mac=macaddr
e1000.vlan=vlan
e1000.netdev=netdev

The strings after the "=" sign come from Property->info->name. So, I
think the attribute "info" is really not needed.

> We need a list o possible values, with a small explanation of each one.
> Do we need the equivalent in json too?

Possible values for "type" are defined in the patch on the
qdev_property_type_to_string() function. To spot them in the current
code, hw/qdev.c:77:

enum PropertyType {
    PROP_TYPE_UNSPEC = 0,
    PROP_TYPE_UINT8,
    PROP_TYPE_UINT16,
    PROP_TYPE_UINT32,
    PROP_TYPE_INT32,
    PROP_TYPE_UINT64,
    PROP_TYPE_TADDR,
    PROP_TYPE_MACADDR,
    PROP_TYPE_DRIVE,
    PROP_TYPE_CHR,
    PROP_TYPE_STRING,
    PROP_TYPE_NETDEV,
    PROP_TYPE_VLAN,
    PROP_TYPE_PTR,
    PROP_TYPE_BIT,
};

So it is a mix of json-(string|integer|boolean). It seams to me that a
device_add using QMP will use just use strings. Need to confirm that.

>
> Suggest a NOTE saying this the equivalent of command-line options -device ?

Ack.

Regards,

Miguel

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Luiz Capitulino]

On Mon, 5 Jul 2010 16:34:22 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> Possible values for "type" are defined in the patch on the
> qdev_property_type_to_string() function. To spot them in the current
> code, hw/qdev.c:77:
> 
> enum PropertyType {
>     PROP_TYPE_UNSPEC = 0,
>     PROP_TYPE_UINT8,
>     PROP_TYPE_UINT16,
>     PROP_TYPE_UINT32,
>     PROP_TYPE_INT32,
>     PROP_TYPE_UINT64,
>     PROP_TYPE_TADDR,
>     PROP_TYPE_MACADDR,
>     PROP_TYPE_DRIVE,
>     PROP_TYPE_CHR,
>     PROP_TYPE_STRING,
>     PROP_TYPE_NETDEV,
>     PROP_TYPE_VLAN,
>     PROP_TYPE_PTR,
>     PROP_TYPE_BIT,
> };
> 
> So it is a mix of json-(string|integer|boolean). It seams to me that a
> device_add using QMP will use just use strings. Need to confirm that.

There are integers too.

Daniel, can you clarify how libvirt is going to use this member?

Maybe we could have something like this:

 "type": { "qdev": "macaddr", "qmp": "string" }

[Qemu-devel] Re: [PATCH 1/2] QMP: Introduce the documentation for query-qdm

[Daniel P. Berrange]

On Wed, Jul 07, 2010 at 10:07:09AM -0300, Luiz Capitulino wrote:
> On Mon, 5 Jul 2010 16:34:22 -0300
> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:
> 
> > Possible values for "type" are defined in the patch on the
> > qdev_property_type_to_string() function. To spot them in the current
> > code, hw/qdev.c:77:
> > 
> > enum PropertyType {
> >     PROP_TYPE_UNSPEC = 0,
> >     PROP_TYPE_UINT8,
> >     PROP_TYPE_UINT16,
> >     PROP_TYPE_UINT32,
> >     PROP_TYPE_INT32,
> >     PROP_TYPE_UINT64,
> >     PROP_TYPE_TADDR,
> >     PROP_TYPE_MACADDR,
> >     PROP_TYPE_DRIVE,
> >     PROP_TYPE_CHR,
> >     PROP_TYPE_STRING,
> >     PROP_TYPE_NETDEV,
> >     PROP_TYPE_VLAN,
> >     PROP_TYPE_PTR,
> >     PROP_TYPE_BIT,
> > };
> > 
> > So it is a mix of json-(string|integer|boolean). It seams to me that a
> > device_add using QMP will use just use strings. Need to confirm that.
> 
> There are integers too.
> 
> Daniel, can you clarify how libvirt is going to use this member?

We're not actively planning to use this field. When I wrote the patch
originally, I was aiming to provide the maximim semantically useful
information possible, rather than just the generic json data type.
This ensures that this is fully self-documenting.


> Maybe we could have something like this:
> 
>  "type": { "qdev": "macaddr", "qmp": "string" }

Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] [PATCH 2/2] monitor: Convert 'info qdm' to QMP

[Miguel Di Ciurcio Filho]

Converts the 'info qdm' command to QMP, allowing the discovery of all devices
known to the QEMU binary without relying on command line paramaters like
-device ? and -device devtype,?

This change does not modify the output of the 'info qdm' monitor command.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 hw/qdev.c |  118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/qdev.h |    3 +-
 monitor.c |    3 +-
 3 files changed, 120 insertions(+), 4 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 61f999c..23f0540 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,6 +29,7 @@
 #include "qdev.h"
 #include "sysemu.h"
 #include "monitor.h"
+#include "qjson.h"
 
 static int qdev_hotplug = 0;
 
@@ -777,13 +778,126 @@ void do_info_qtree(Monitor *mon)
         qbus_print(mon, main_system_bus, 0);
 }
 
-void do_info_qdm(Monitor *mon)
+static void qdm_list_iter(QObject *obj, void *opaque)
+{
+
+    Monitor *mon = opaque;
+    QDict *dev = qobject_to_qdict(obj);
+
+    monitor_printf(mon, "name \"%s\", bus %s", qdict_get_str(dev, "name"),
+            qdict_get_str(dev, "bus"));
+
+    if (qdict_haskey(dev, "alias")) {
+        monitor_printf(mon, ", alias \"%s\"", qdict_get_str(dev, "alias"));
+    }
+
+    if (qdict_haskey(dev, "description")) {
+        monitor_printf(mon, ", desc \"%s\"", qdict_get_str(dev, "description"));
+    }
+
+    if (!qdict_get_bool(dev, "creatable")) {
+        monitor_printf(mon, ", no-user");
+    }
+
+    monitor_printf(mon, "\n");
+}
+
+void do_info_qdm_print(Monitor *mon, const QObject *ret_data)
+{
+    QList *devs;
+
+    devs = qobject_to_qlist(ret_data);
+    qlist_iter(devs, qdm_list_iter, mon);
+}
+
+static const char *qdev_property_type_to_string(int type) {
+    switch (type) {
+    case PROP_TYPE_UNSPEC:
+	    return "unknown";
+    case PROP_TYPE_UINT8:
+	    return "uint8";
+    case PROP_TYPE_UINT16:
+	    return "uint16";
+    case PROP_TYPE_UINT32:
+	    return "uint32";
+    case PROP_TYPE_INT32:
+	    return "int32";
+    case PROP_TYPE_UINT64:
+	    return "uint64";
+    case PROP_TYPE_TADDR:
+	    return "taddr";
+    case PROP_TYPE_MACADDR:
+	    return "macaddr";
+    case PROP_TYPE_DRIVE:
+	    return "drive";
+    case PROP_TYPE_CHR:
+	    return "chr";
+    case PROP_TYPE_STRING:
+	    return "string";
+    case PROP_TYPE_NETDEV:
+	    return "netdev";
+    case PROP_TYPE_VLAN:
+	    return "vlan";
+    case PROP_TYPE_PTR:
+	    return "pointer";
+    case PROP_TYPE_BIT:
+	    return "bit";
+    }
+
+    return NULL;
+}
+
+void do_info_qdm(Monitor *mon, QObject **ret_data)
 {
     DeviceInfo *info;
+    QList *devs = qlist_new();
 
     for (info = device_info_list; info != NULL; info = info->next) {
-        qdev_print_devinfo(info);
+        QObject *obj;
+        QDict *dev;
+        QList *props = qlist_new();
+        Property *prop;
+
+        for (prop = info->props; prop && prop->name; prop++) {
+            QObject *entry;
+            /*
+             * TODO Properties without a parser are just for dirty hacks.
+             * qdev_prop_ptr is the only such PropertyInfo.  It's marked
+             * for removal.  This conditional should be removed along with
+             * it.
+             */
+            if (!prop->info->parse) {
+                continue;           /* no way to set it, don't show */
+            }
+
+            const char *type = qdev_property_type_to_string(prop->info->type);
+
+            entry = qobject_from_jsonf("{ 'name': %s, 'info': %s, 'type': %s }",
+                           prop->name, prop->info->name, type);
+
+            qlist_append_obj(props, entry);
+        }
+
+        obj = qobject_from_jsonf("{ 'name': %s, 'bus': %s, 'props': %p, 'creatable': %i }",
+                     info->name,
+                     info->bus_info->name,
+                     props,
+                     info->no_user ? 0 : 1);
+
+        dev = qobject_to_qdict(obj);
+
+        if (info->alias) {
+            qdict_put(dev, "alias", qstring_from_str(info->alias));
+        }
+
+        if (info->desc) {
+            qdict_put(dev, "description", qstring_from_str(info->desc));
+        }
+
+        qlist_append(devs, dev);
     }
+
+    *ret_data = QOBJECT(devs);
 }
 
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data)
diff --git a/hw/qdev.h b/hw/qdev.h
index be5ad67..f44e4a2 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -181,7 +181,8 @@ void qbus_free(BusState *bus);
 /*** monitor commands ***/
 
 void do_info_qtree(Monitor *mon);
-void do_info_qdm(Monitor *mon);
+void do_info_qdm_print(Monitor *mon, const QObject *ret_data);
+void do_info_qdm(Monitor *mon, QObject **ret_data);
 int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_device_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
diff --git a/monitor.c b/monitor.c
index 170b269..91b87f9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2550,7 +2550,8 @@ static const mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show qdev device model list",
-        .mhandler.info = do_info_qdm,
+        .user_print = do_info_qdm_print,
+        .mhandler.info_new = do_info_qdm,
     },
     {
         .name       = "roms",
-- 
1.7.1