[Qemu-devel] Re: [PATCH v3 2/2] monitor: Convert 'info qdm' to QMP
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
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
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
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