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

[Luiz Capitulino <lcapitulino@redhat.com>]

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",