On 04/13/2018 03:05 AM, Perez Blanco, Ricardo (Nokia - BE/Antwerp) wrote:
> Dear all,
> 
> Here you can find my first contribution to qemu. Please, do not hesitate to do any kind of remark.

Welcome to the community.  Looking forward to your v2 patch submission
(see my reply to your followup, for more things to fix before you send v2).


> Subject: [PATCH] Show values and description when using "qom-list"
> 
> For debugging purposes it is very useful to:
>  - See the description of the field. This information is already filled
>    in but not shown in "qom-list" command.
>  - Display value of the field. So far, only well known types are
>    implemented (string, str, int, uint, bool).
> 
> Signed-off-by: Ricardo Perez Blanco <ricardo.perez_blanco@nokia.com>
> ---
>  hmp.c          | 13 +++++++++++--
>  qapi/misc.json |  4 +++-
>  qmp.c          | 26 ++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index a25c7bd..967e0b2 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2490,8 +2490,17 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
>          while (list != NULL) {
>              ObjectPropertyInfo *value = list->value;
> 
> -            monitor_printf(mon, "%s (%s)\n",
> -                           value->name, value->type);
> +            monitor_printf(mon, "%s", value->name);
> +            if (value->value) {
> +                monitor_printf(mon, "=%s", value->value);
> +            }

Technically, you should be checking 'if (value->has_value)'.  It happens
that our current code sets value->value to NULL if value->has_value is
false, but that's less reliable.  Someday, we may improve our QAPI code
generator to quit generating has_FOO members when FOO is an optional
pointer and NULL is an obvious indication that FOO was not provided (at
which point, your code as written is correct), but we're not there yet.


> +++ b/qapi/misc.json
> @@ -1328,10 +1328,12 @@
>  #
>  # @description: if specified, the description of the property.
>  #
> +# @value: if specified, the value of the property.

Missing a mention that this field is '(since 2.13)'.

> +#
>  # Since: 1.2
>  ##
>  { 'struct': 'ObjectPropertyInfo',
> -  'data': { 'name': 'str', 'type': 'str', '*description': 'str' } }
> +  'data': { 'name': 'str', 'type': 'str', '*description':'str', '*value':'str' } }
> 
>  ##
>  # @qom-list:
> diff --git a/qmp.c b/qmp.c
> index f722616..750b5d0 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -237,6 +237,32 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
> 
>          entry->value->name = g_strdup(prop->name);
>          entry->value->type = g_strdup(prop->type);
> +        if (prop->description) {
> +            entry->value->description = g_strdup(prop->description);
> +        }
> +        if ((g_ascii_strncasecmp(entry->value->type, "string", 6) == 0) ||
> +            (g_ascii_strncasecmp(entry->value->type, "str", 3) == 0)) {

This will accept a type named "strange"; is that intentional?

> +            Error **errp = NULL;
> +            entry->value->value = g_strdup_printf("\"%s\"",
> +                object_property_get_str(obj, entry->value->name, errp));
> +        }
> +        if (g_ascii_strncasecmp(entry->value->type, "int", 3) == 0) {

Likewise, this will accept a type named "internal".  I'm not sure if
your manual checking for types by names is the best approach; and the
fact that we are stringizing the result instead of using the natural
JSON type (a string for "str", a number for "int") is a bit worrisome.

> +            Error **errp = NULL;
> +            entry->value->value = g_strdup_printf("%ld",
> +                object_property_get_int(obj, entry->value->name, errp));
> +        }
> +        if (g_ascii_strncasecmp(entry->value->type, "uint", 4) == 0) {
> +            Error **errp = NULL;
> +            entry->value->value = g_strdup_printf("%lu",
> +                object_property_get_uint(obj, entry->value->name, errp));
> +        }
> +        if (g_ascii_strncasecmp(entry->value->type, "bool", 4) == 0) {
> +            Error **errp = NULL;
> +            entry->value->value = g_strdup_printf("%s",
> +               (object_property_get_bool(obj, entry->value->name, errp) == true)
> +                ? "true" : "false");
> +        }

Since the ->value field is optional, you have to set
entry->value->has_value = true anywhere that you are setting
entry->value->value to a string.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org