Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Luiz Capitulino]

On Sun, 27 Apr 2014 11:29:30 +0100
Hani Benhabiles <kroosec@gmail.com> wrote:

> Signed-off-by: Hani Benhabiles <hani@linux.com>

Who can review this one? I'm not familiar enough with QOM to do it.

> ---
> 
> Not sure whether the qobject stringifying functions could fit or be of some use
> elsewhere. Thus, I kept them as static near the only place where they are used
> at the moment.
> 
>  hmp-commands.hx |   2 +
>  hmp.c           | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h           |   1 +
>  monitor.c       |   7 +++
>  4 files changed, 153 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f3fc514..65b5e33 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1754,6 +1754,8 @@ show qdev device model list
>  show roms
>  @item info tpm
>  show the TPM device
> +@item info qom-tree
> +show the QOM tree
>  @end table
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 2f279c4..891a34e 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -22,9 +22,11 @@
>  #include "qemu/sockets.h"
>  #include "monitor/monitor.h"
>  #include "qapi/opts-visitor.h"
> +#include "qapi/qmp/types.h"
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "qom/qom-qobject.h"
>  
>  static void hmp_handle_error(Monitor *mon, Error **errp)
>  {
> @@ -712,6 +714,147 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
>      qapi_free_TPMInfoList(info_list);
>  }
>  
> +static char *qobject_to_str(const QObject *, int);
> +
> +static char *qlist_to_str(const QObject *obj, int indent)
> +{
> +    char *buf = NULL;
> +    const QListEntry *entry;
> +    int i = 0;
> +
> +    assert(qobject_type(obj) == QTYPE_QLIST);
> +
> +    entry = qlist_first(qobject_to_qlist(obj));
> +    while (entry) {
> +        char *new, *str;
> +        int type = qobject_type(entry->value);
> +        str = qobject_to_str(entry->value, indent);
> +        if (type == QTYPE_QLIST || type == QTYPE_QDICT) {
> +            new = g_strdup_printf("%s%*sElement #%d:\n%s", buf ?: "",
> +                                  indent, "", i++, str);
> +        } else {
> +            new = g_strdup_printf("%s%*sElement #%d: %s\n", buf ?: "",
> +                                  indent, "", i++, str);
> +        }
> +        g_free(buf);
> +        g_free(str);
> +        buf = new;
> +        entry = qlist_next(entry);
> +    }
> +    return buf;
> +}
> +
> +static char *qdict_to_str(const QObject *obj, int indent)
> +{
> +    QDict *dict = qobject_to_qdict(obj);
> +    const QDictEntry *entry;
> +    char *buf = NULL;
> +
> +    assert(qobject_type(obj) == QTYPE_QDICT);
> +
> +    for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
> +        char *str, *new;
> +        int type = qobject_type(entry->value);
> +        str = qobject_to_str(entry->value, indent);
> +        if (type == QTYPE_QLIST || type == QTYPE_QDICT) {
> +            new = g_strdup_printf("%s%*s%s:\n%s", buf ?: "", indent, "",
> +                                  entry->key, str);
> +        } else {
> +            new = g_strdup_printf("%s%*s%s: %s\n", buf ?: "", indent, "",
> +                                  entry->key, str);
> +        }
> +        g_free(buf);
> +        g_free(str);
> +        buf = new;
> +    }
> +    return buf;
> +}
> +
> +static char *qobject_to_str(const QObject *obj, int indent)
> +{
> +    switch (qobject_type(obj)) {
> +    case QTYPE_QSTRING: {
> +        QString *value = qobject_to_qstring(obj);
> +        return g_strdup(qstring_get_str(value));
> +    }
> +    case QTYPE_QINT: {
> +        QInt *value = qobject_to_qint(obj);
> +        return g_strdup_printf("%" PRId64, qint_get_int(value));
> +    }
> +    case QTYPE_QBOOL: {
> +        QBool *value = qobject_to_qbool(obj);
> +        return g_strdup(qbool_get_int(value) ? "True" : "False");
> +    }
> +    case QTYPE_QERROR: {
> +        QString *value = qerror_human((QError *)obj);
> +        return g_strdup(qstring_get_str(value));
> +    }
> +    case QTYPE_QFLOAT: {
> +        QFloat *value = qobject_to_qfloat(obj);
> +        return g_strdup_printf("%g", qfloat_get_double(value));
> +    }
> +    case QTYPE_QLIST:
> +        return qlist_to_str(obj, indent + 2);
> +    case QTYPE_QDICT:
> +        return qdict_to_str(obj, indent + 2);
> +    case QTYPE_NONE:
> +        break;
> +    case QTYPE_MAX:
> +    default:
> +        abort();
> +    }
> +    return NULL;
> +}
> +
> +static void hmp_print_qom_tree(Monitor *mon, const char *path, int indent)
> +{
> +    ObjectPropertyInfoList *list, *start;
> +
> +    monitor_printf(mon, "%*s%s:\n", indent, "", path);
> +    start = list = qmp_qom_list(path, NULL);
> +    indent += 2;
> +    while (list) {
> +        ObjectPropertyInfo *info = list->value;
> +
> +        if (!strncmp(info->type, "child<", 5)) {
> +            char *name = g_strdup_printf("%s/%s", path, info->name);
> +            hmp_print_qom_tree(mon, name, indent);
> +            g_free(name);
> +        } else {
> +            Object *obj = NULL;
> +            QObject *data = NULL;
> +            char *str;
> +
> +            obj = object_resolve_path(path, NULL);
> +            if (!obj) {
> +                list = list->next;
> +                continue;
> +            }
> +            data = object_property_get_qobject(obj, info->name, NULL);
> +            if (!data) {
> +                list = list->next;
> +                continue;
> +            }
> +            str = qobject_to_str(data, indent);
> +            if (qobject_type(data) == QTYPE_QDICT
> +                || qobject_type(data) == QTYPE_QLIST) {
> +                monitor_printf(mon, "%*s%s:\n%s", indent, "", info->name, str);
> +            } else {
> +                monitor_printf(mon, "%*s%s: %s\n", indent, "", info->name, str);
> +            }
> +            g_free(str);
> +            qobject_decref(data);
> +        }
> +        list = list->next;
> +    }
> +    qapi_free_ObjectPropertyInfoList(start);
> +}
> +
> +void hmp_info_qom_tree(Monitor *mon, const QDict *qdict)
> +{
> +    hmp_print_qom_tree(mon, "/machine", 0);
> +}
> +
>  void hmp_quit(Monitor *mon, const QDict *qdict)
>  {
>      monitor_suspend(mon);
> diff --git a/hmp.h b/hmp.h
> index ed58f0e..1733242 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);
> +void hmp_info_qom_tree(Monitor *mon, const QDict *qdict);
>  void hmp_quit(Monitor *mon, const QDict *qdict);
>  void hmp_stop(Monitor *mon, const QDict *qdict);
>  void hmp_system_reset(Monitor *mon, const QDict *qdict);
> diff --git a/monitor.c b/monitor.c
> index 342e83b..fcb1d0c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2966,6 +2966,13 @@ static mon_cmd_t info_cmds[] = {
>          .mhandler.cmd = hmp_info_tpm,
>      },
>      {
> +        .name       = "qom-tree",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show the QOM tree",
> +        .mhandler.cmd = hmp_info_qom_tree,
> +    },
> +    {
>          .name       = NULL,
>      },
>  };

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Paolo Bonzini]

Il 05/05/2014 20:33, Luiz Capitulino ha scritto:
>> +            data = object_property_get_qobject(obj, info->name, NULL);
>> +            if (!data) {
>> +                list = list->next;
>> +                continue;

You could use object_property_print and get rid of a relatively large 
amount of code.

Apart from this, the patch is definitely useful.

Paolo

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Hani Benhabiles]

On Wed, May 07, 2014 at 01:01:12PM +0200, Paolo Bonzini wrote:
> Il 05/05/2014 20:33, Luiz Capitulino ha scritto:
> >>+            data = object_property_get_qobject(obj, info->name, NULL);
> >>+            if (!data) {
> >>+                list = list->next;
> >>+                continue;
> 
> You could use object_property_print and get rid of a relatively large amount
> of code.

Thanks for pointing that. But StringOutputVisitor doesn't have handlers for structs and
lists like QMP countrepart so the call segfaults when used against object
properties using that.

ie. Unless I am missing something, I should add those handlers to the string
visitor before being able to use object_property_print(), right ?

> Apart from this, the patch is definitely useful.

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Andreas Färber]

Hi Hani,

Am 27.04.2014 12:29, schrieb Hani Benhabiles:
> Signed-off-by: Hani Benhabiles <hani@linux.com>
> ---
> 
> Not sure whether the qobject stringifying functions could fit or be of some use
> elsewhere. Thus, I kept them as static near the only place where they are used
> at the moment.

Your "info qom-tree" reads quite similar to my proposed qom-tree script:
http://patchwork.ozlabs.org/patch/317224/

For HMP I had been working on a command "info qom-composition" that
emits a trimmed-down overview-style output, such as for x86_64:

(qemu) info qom-composition
/machine (pc-i440fx-2.1-machine)
  /peripheral-anon (container)
  /peripheral (container)
  /unattached (container)
    /sysbus (System)
    /device[0] (qemu64-x86_64-cpu)
      /apic (apic)
    /device[1] (kvmvapic)
    /device[2] (i440FX)
    /device[3] (PIIX3)
      /isa.0 (ISA)
    /device[4] (isa-i8259)
    /device[5] (isa-i8259)
    /device[6] (cirrus-vga)
    /device[7] (hpet)
    /device[8] (mc146818rtc)
    /device[9] (isa-pit)
    /device[10] (isa-pcspk)
    /device[11] (isa-serial)
    /device[12] (isa-parallel)
    /device[13] (i8042)
    /device[14] (vmport)
    /device[15] (vmmouse)
    /device[16] (port92)
    /device[17] (isa-fdc)
    /device[18] (e1000)
    /device[19] (piix3-ide)
      /ide.0 (IDE)
      /ide.1 (IDE)
    /device[20] (ide-cd)
    /device[21] (PIIX4_PM)
      /i2c (i2c-bus)
    /device[22] (smbus-eeprom)
    /device[23] (smbus-eeprom)
    /device[24] (smbus-eeprom)
    /device[25] (smbus-eeprom)
    /device[26] (smbus-eeprom)
    /device[27] (smbus-eeprom)
    /device[28] (smbus-eeprom)
    /device[29] (smbus-eeprom)
  /icc-bridge (icc-bridge)
    /icc (icc-bus)
  /fw_cfg (fw_cfg)
  /i440fx (i440FX-pcihost)
    /pci.0 (PCI)
    /ioapic (ioapic)

I believe both commands can coexist. Question is how.

My tree-walking implementation is not based on QMP and thus much
slimmer. I didn't have to care about printing properties yet - that I
deferred to a qom-get HMP command (which based on previous feedback we
should implement either way). Possibly we could rebase your patch onto
mine, adding an argument for whether or not to print the properties?

Compared to my script, your "info qom-tree" does not allow to change the
root, so I believe we would need to print from "/" on (rather than
"/machine"), for dumping RNG backends etc. as well. The alternative
would be having arguments to the command, for specifying root and/or
output style.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] monitor: Add info qom-tree subcommand.

[Hani Benhabiles]

On Wed, May 07, 2014 at 05:39:36PM +0200, Andreas Färber wrote:
> Hi Hani,
> 
> Am 27.04.2014 12:29, schrieb Hani Benhabiles:
> > Signed-off-by: Hani Benhabiles <hani@linux.com>
> > ---
> > 
> > Not sure whether the qobject stringifying functions could fit or be of some use
> > elsewhere. Thus, I kept them as static near the only place where they are used
> > at the moment.
> 
> Your "info qom-tree" reads quite similar to my proposed qom-tree script:
> http://patchwork.ozlabs.org/patch/317224/
> 
> For HMP I had been working on a command "info qom-composition" that
> emits a trimmed-down overview-style output, such as for x86_64:
> 
> (qemu) info qom-composition
> /machine (pc-i440fx-2.1-machine)
>   /peripheral-anon (container)
>   /peripheral (container)
>   /unattached (container)
>     /sysbus (System)
>     /device[0] (qemu64-x86_64-cpu)
>       /apic (apic)
>     /device[1] (kvmvapic)
>     /device[2] (i440FX)
>     /device[3] (PIIX3)
>       /isa.0 (ISA)
>     /device[4] (isa-i8259)
>     /device[5] (isa-i8259)
>     /device[6] (cirrus-vga)
>     /device[7] (hpet)
>     /device[8] (mc146818rtc)
>     /device[9] (isa-pit)
>     /device[10] (isa-pcspk)
>     /device[11] (isa-serial)
>     /device[12] (isa-parallel)
>     /device[13] (i8042)
>     /device[14] (vmport)
>     /device[15] (vmmouse)
>     /device[16] (port92)
>     /device[17] (isa-fdc)
>     /device[18] (e1000)
>     /device[19] (piix3-ide)
>       /ide.0 (IDE)
>       /ide.1 (IDE)
>     /device[20] (ide-cd)
>     /device[21] (PIIX4_PM)
>       /i2c (i2c-bus)
>     /device[22] (smbus-eeprom)
>     /device[23] (smbus-eeprom)
>     /device[24] (smbus-eeprom)
>     /device[25] (smbus-eeprom)
>     /device[26] (smbus-eeprom)
>     /device[27] (smbus-eeprom)
>     /device[28] (smbus-eeprom)
>     /device[29] (smbus-eeprom)
>   /icc-bridge (icc-bridge)
>     /icc (icc-bus)
>   /fw_cfg (fw_cfg)
>   /i440fx (i440FX-pcihost)
>     /pci.0 (PCI)
>     /ioapic (ioapic)
> 
> I believe both commands can coexist. Question is how.
> 
> My tree-walking implementation is not based on QMP and thus much
> slimmer. I didn't have to care about printing properties yet - that I
> deferred to a qom-get HMP command (which based on previous feedback we
> should implement either way). Possibly we could rebase your patch onto
> mine, adding an argument for whether or not to print the properties?
>

Sure, I am fine with rebasing the patch on top of yours if that ends up with
less duplicated work.

> Compared to my script, your "info qom-tree" does not allow to change the
> root, so I believe we would need to print from "/" on (rather than
> "/machine"), for dumping RNG backends etc. as well. The alternative
> would be having arguments to the command, for specifying root and/or
> output style.

A path argument (defaulting to "/") seems like a good approach to me, but I have
no strong preference on this.