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

2010-07-21 Thread Anthony Liguori

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





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

2010-07-21 Thread Luiz Capitulino
On Wed, 21 Jul 2010 18:51:19 +0100
"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  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 
> > > 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.



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

2010-07-21 Thread Daniel P. Berrange
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  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 
> > 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
-- 
|: 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] Re: [PATCH v3 2/2] monitor: Convert 'info qdm' to QMP

2010-07-21 Thread Luiz Capitulino
On Mon, 19 Jul 2010 13:51:27 -0300
Miguel Di Ciurcio Filho  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 
> ---
>  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);
> -voi