Patchwork [v3,2/2] monitor: Convert 'info qdm' to QMP

login
register
mail settings
Submitter Miguel Di Ciurcio Filho
Date July 19, 2010, 4:51 p.m.
Message ID <1279558287-9446-3-git-send-email-miguel.filho@gmail.com>
Download mbox | patch
Permalink /patch/59209/
State New
Headers show

Comments

Miguel Di Ciurcio Filho - July 19, 2010, 4:51 p.m.
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 |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/qdev.h |    3 +-
 monitor.c |    3 +-
 3 files changed, 112 insertions(+), 4 deletions(-)
Luiz Capitulino - July 21, 2010, 5:42 p.m.
On Mon, 19 Jul 2010 13:51:27 -0300
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:

> 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 |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/qdev.h |    3 +-
>  monitor.c |    3 +-
>  3 files changed, 112 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev.c b/hw/qdev.c
> index e99c73f..d24d42a 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;
>  
> @@ -779,13 +780,118 @@ 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_UINT8:
> +    case PROP_TYPE_UINT16:
> +    case PROP_TYPE_UINT32:
> +    case PROP_TYPE_INT32:
> +    case PROP_TYPE_UINT64:
> +        return "integer";
> +    case PROP_TYPE_TADDR:
> +    case PROP_TYPE_MACADDR:
> +    case PROP_TYPE_DRIVE:
> +    case PROP_TYPE_CHR:
> +    case PROP_TYPE_STRING:
> +    case PROP_TYPE_NETDEV:
> +        return "string";
> +    case PROP_TYPE_BIT:
> +	    return "boolean";
> +    case PROP_TYPE_UNSPEC:
> +    case PROP_TYPE_VLAN:
> +    case PROP_TYPE_PTR:
> +        return NULL;

Shouldn't you just drop UNSPEC, VLAN and PRT?

> +    }
> +
> +    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: skip old and hackish stuff, they will be removed some day.
> +             */
> +            if (!prop->info->parse || prop->info->type == PROP_TYPE_VLAN
> +                || prop->info->type == PROP_TYPE_PTR
> +                || prop->info->type == PROP_TYPE_UNSPEC) {
> +                continue;
> +            }
> +
> +            const char *type = qdev_property_type_to_string(prop->info->type);

Variable definitions should be on the top and that function can return
NULL. You should put an assert() here if that's impossible to happen,
otherwise qdev_property_type_to_string() should return "unknown" and that
should be documented.

I think the assert() is fine.

> +
> +            entry = qobject_from_jsonf("{ 'name': %s, 'type': %s }",
> +                           prop->name, type);
> +
> +            qlist_append_obj(props, entry);
> +        }
> +
> +        obj = qobject_from_jsonf("{ 'name': %s, 'bus': %s, 'creatable': %i }",
> +                     info->name,
> +                     info->bus_info->name,
> +                     info->no_user ? 0 : 1);
> +
> +        dev = qobject_to_qdict(obj);
> +
> +        if (!qlist_empty(props)) {
> +            qdict_put(dev, "properties", props);
> +        }

We'll leak 'props' when it's empty.

> +
> +        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 678f8b7..3b0382b 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -184,7 +184,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 45fd482..66810f2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2565,7 +2565,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,

Haven't we agreed on calling this query-available-devices or something
like that?

>      },
>      {
>          .name       = "roms",
Daniel P. Berrange - July 21, 2010, 5:51 p.m.
On Wed, Jul 21, 2010 at 02:42:28PM -0300, Luiz Capitulino wrote:
> On Mon, 19 Jul 2010 13:51:27 -0300
> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:
> 
> > 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>
> > diff --git a/monitor.c b/monitor.c
> > index 45fd482..66810f2 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2565,7 +2565,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,
> 
> Haven't we agreed on calling this query-available-devices or something
> like that?

That's getting rather verbose for a name ! How about just
'query-dev-types' (anticipating future query-netdev-types,
query-chardev-types, commands etc, too)


Daniel
Luiz Capitulino - July 21, 2010, 6:20 p.m.
On Wed, 21 Jul 2010 18:51:19 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, Jul 21, 2010 at 02:42:28PM -0300, Luiz Capitulino wrote:
> > On Mon, 19 Jul 2010 13:51:27 -0300
> > Miguel Di Ciurcio Filho <miguel.filho@gmail.com> wrote:
> > 
> > > 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>
> > > diff --git a/monitor.c b/monitor.c
> > > index 45fd482..66810f2 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -2565,7 +2565,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,
> > 
> > Haven't we agreed on calling this query-available-devices or something
> > like that?
> 
> That's getting rather verbose for a name ! How about just
> 'query-dev-types' (anticipating future query-netdev-types,
> query-chardev-types, commands etc, too)

I don't mind long names in the protocol, but I'm ok with your suggestion.
Anthony Liguori - July 21, 2010, 6:30 p.m.
On 07/21/2010 12:51 PM, Daniel P. Berrange wrote:
> On Wed, Jul 21, 2010 at 02:42:28PM -0300, Luiz Capitulino wrote:
>    
>> On Mon, 19 Jul 2010 13:51:27 -0300
>> Miguel Di Ciurcio Filho<miguel.filho@gmail.com>  wrote:
>>
>>      
>>> 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>
>>> diff --git a/monitor.c b/monitor.c
>>> index 45fd482..66810f2 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -2565,7 +2565,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,
>>>        
>> Haven't we agreed on calling this query-available-devices or something
>> like that?
>>      
> That's getting rather verbose for a name ! How about just
> 'query-dev-types' (anticipating future query-netdev-types,
> query-chardev-types, commands etc, too)
>    

I know we're still trying to recover from the great bit shortage of '09 
but I think we can afford to spare some here to make the names readable :-)

Good names are part of good documentation.  If it's not entirely obvious 
what the function does from it's name, it's probably a bad name (in the 
context of a wire protocol).

Regards,

Anthony Liguori

>
> Daniel
>

Patch

diff --git a/hw/qdev.c b/hw/qdev.c
index e99c73f..d24d42a 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;
 
@@ -779,13 +780,118 @@  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_UINT8:
+    case PROP_TYPE_UINT16:
+    case PROP_TYPE_UINT32:
+    case PROP_TYPE_INT32:
+    case PROP_TYPE_UINT64:
+        return "integer";
+    case PROP_TYPE_TADDR:
+    case PROP_TYPE_MACADDR:
+    case PROP_TYPE_DRIVE:
+    case PROP_TYPE_CHR:
+    case PROP_TYPE_STRING:
+    case PROP_TYPE_NETDEV:
+        return "string";
+    case PROP_TYPE_BIT:
+	    return "boolean";
+    case PROP_TYPE_UNSPEC:
+    case PROP_TYPE_VLAN:
+    case PROP_TYPE_PTR:
+        return NULL;
+    }
+
+    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: skip old and hackish stuff, they will be removed some day.
+             */
+            if (!prop->info->parse || prop->info->type == PROP_TYPE_VLAN
+                || prop->info->type == PROP_TYPE_PTR
+                || prop->info->type == PROP_TYPE_UNSPEC) {
+                continue;
+            }
+
+            const char *type = qdev_property_type_to_string(prop->info->type);
+
+            entry = qobject_from_jsonf("{ 'name': %s, 'type': %s }",
+                           prop->name, type);
+
+            qlist_append_obj(props, entry);
+        }
+
+        obj = qobject_from_jsonf("{ 'name': %s, 'bus': %s, 'creatable': %i }",
+                     info->name,
+                     info->bus_info->name,
+                     info->no_user ? 0 : 1);
+
+        dev = qobject_to_qdict(obj);
+
+        if (!qlist_empty(props)) {
+            qdict_put(dev, "properties", props);
+        }
+
+        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 678f8b7..3b0382b 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -184,7 +184,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 45fd482..66810f2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2565,7 +2565,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",