Re: [Qemu-devel] [PATCH v2] object: Add 'help' option for all available backends and properties

[Markus Armbruster <armbru@redhat.com>]

Lin Ma <lma@suse.com> writes:

> '-object help' prints available user creatable backends.
> '-object $typename,help' prints relevant properties.
>
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  include/qom/object_interfaces.h |   2 +
>  qemu-options.hx                 |   7 ++-
>  qom/object_interfaces.c         | 112 ++++++++++++++++++++++++++++++++++++++++
>  vl.c                            |   5 ++
>  4 files changed, 125 insertions(+), 1 deletion(-)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 8b17f4d..197cd5f 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -165,4 +165,6 @@ int user_creatable_add_opts_foreach(void *opaque,
>   */
>  void user_creatable_del(const char *id, Error **errp);
>  
> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp);
> +
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index a71aaf8..fade53c 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3752,7 +3752,9 @@ DEF("object", HAS_ARG, QEMU_OPTION_object,
>      "                create a new object of type TYPENAME setting properties\n"
>      "                in the order they are specified.  Note that the 'id'\n"
>      "                property must be set.  These objects are placed in the\n"
> -    "                '/objects' path.\n",
> +    "                '/objects' path.\n"
> +    "                Use '-object help' to print available backend types and\n"
> +    "                '-object typename,help' to print relevant properties.\n",
>      QEMU_ARCH_ALL)
>  STEXI
>  @item -object @var{typename}[,@var{prop1}=@var{value1},...]
> @@ -3762,6 +3764,9 @@ in the order they are specified.  Note that the 'id'
>  property must be set.  These objects are placed in the
>  '/objects' path.
>  
> +Use '-object help' to print available backend types and
> +'-object typename,help' to print relevant properties.
> +

Make that

  +Use @code{-object help} to print available backend types and
  +@code{-object @var{typename},help} to print relevant properties.
  +

>  @table @option
>  
>  @item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off}
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index bf59846..4ee8643 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -5,6 +5,7 @@
>  #include "qapi-visit.h"
>  #include "qapi/qmp-output-visitor.h"
>  #include "qapi/opts-visitor.h"
> +#include "qemu/help_option.h"
>  
>  void user_creatable_complete(Object *obj, Error **errp)
>  {
> @@ -212,6 +213,117 @@ void user_creatable_del(const char *id, Error **errp)
>      object_unparent(obj);
>  }
>  
> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    Visitor *v;
> +    char *type = NULL;
> +    Error *local_err = NULL;
> +

Why this blank line?

> +    int i;
> +    char *values = NULL;
> +    Object *obj;
> +    ObjectPropertyInfoList *props = NULL;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +    ObjectPropertyInfoList *start;
> +
> +    struct EnumProperty {
> +        const char * const *strings;
> +        int (*get)(Object *, Error **);
> +        void (*set)(Object *, int, Error **);
> +    } *enumprop;

Copied from object.c.  Big no-no.  Whatever you do with this probably
belongs into object.c instead.

> +
> +    v = opts_visitor_new(opts);
> +    visit_start_struct(v, NULL, NULL, 0, &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    visit_type_str(v, "qom-type", &type, &local_err);
> +    if (local_err) {
> +        goto out_visit;
> +    }
> +
> +    if (type && is_help_option(type)) {

I think the Options visitor is overkill.  Much simpler:

       type = qemu_opt_get(opts, "qom-type");
       if (type && is_help_option(type)) {

> +        printf("Available object backend types:\n");
> +        GSList *list = object_class_get_list(TYPE_USER_CREATABLE, false);
> +        while (list) {
> +            const char *name;
> +            name = object_class_get_name(OBJECT_CLASS(list->data));
> +            if (strcmp(name, TYPE_USER_CREATABLE)) {

Why do you need to skip TYPE_USER_CREATABLE?

Hmm...

    $ qemu-system-x86_64 -object user-creatable,id=foo
    **
    ERROR:/work/armbru/qemu/qom/object.c:361:object_initialize_with_type: assertion failed (type->instance_size >= sizeof(Object)): (0 >= 40)
    Aborted (core dumped)

Should this type be abstract?

> +                printf("%s\n", name);
> +            }
> +            list = list->next;
> +        }

Recommend to keep the loop control in one place:

           for (list = object_class_get_list(TYPE_USER_CREATABLE, false);
                list;
                list = list->next) {
               ...
           }

If you hate multi-line for (...), you can do

           GSList *head = object_class_get_list(TYPE_USER_CREATABLE, false);

           for (list = head; list; list->next) {
               ...
           }

> +        g_slist_free(list);
> +        goto out_visit;
> +    }
> +
> +    if (!type || !qemu_opt_has_help_opt(opts)) {
> +        visit_end_struct(v, NULL);
> +        return 0;
> +    }
> +
> +    if (!object_class_by_name(type)) {
> +        printf("invalid object type: %s\n", type);
> +        goto out_visit;
> +    }
> +    obj = object_new(type);
> +    object_property_iter_init(&iter, obj);
> +
> +    while ((prop = object_property_iter_next(&iter))) {
> +        ObjectPropertyInfoList *entry = g_malloc0(sizeof(*entry));

Blank line between declarations and statements, please.

> +        entry->value = g_malloc0(sizeof(ObjectPropertyInfo));

Either g_malloc0(sizeof(entry->value), or g_new0(ObjectPropertyInfo).

> +        entry->next = props;
> +        props = entry;
> +        entry->value->name = g_strdup(prop->name);
> +        i = 0;
> +        enumprop = prop->opaque;
> +        if (!g_str_equal(prop->type, "string") && \
> +            !g_str_equal(prop->type, "bool") && \
> +            !g_str_equal(prop->type, "struct tm") && \
> +            !g_str_equal(prop->type, "int") && \
> +            !g_str_equal(prop->type, "uint8") && \
> +            !g_str_equal(prop->type, "uint16") && \
> +            !g_str_equal(prop->type, "uint32") && \
> +            !g_str_equal(prop->type, "uint64")) {

Uh, what are you trying to test with this condition?  It's not obvious
to me...

> +            while (enumprop->strings[i] != NULL) {
> +                if (i != 0) {
> +                    values = g_strdup_printf("%s/%s",
> +                                            values, enumprop->strings[i]);
> +                } else {
> +                    values = g_strdup_printf("%s", enumprop->strings[i]);
> +                }
> +                i++;
> +            }
> +            entry->value->type = g_strdup_printf("%s, available values: %s",
> +                                                prop->type, values);

Looks like this is the enum case.  But why is the condition true exactly
for enums?

> +        } else {
> +            entry->value->type = g_strdup(prop->type);
> +        }
> +    }

The loop above collects properties, and ...

> +
> +    start = props;
> +    while (props != NULL) {
> +        ObjectPropertyInfo *value = props->value;
> +        printf("%s (%s)\n", value->name, value->type);
> +        props = props->next;
> +    }
> +    qapi_free_ObjectPropertyInfoList(start);

... this loop prints them.  Have you considered fusing the two loops?

> +
> +out_visit:
> +    visit_end_struct(v, NULL);
> +
> +out:
> +    g_free(values);
> +    g_free(type);
> +    object_unref(obj);
> +    if (local_err) {
> +        error_report_err(local_err);
> +    }
> +    return 1;
> +}
> +
>  static void register_types(void)
>  {
>      static const TypeInfo uc_interface_info = {
> diff --git a/vl.c b/vl.c
> index ee557a1..a2230c8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4251,6 +4251,11 @@ int main(int argc, char **argv, char **envp)
>      page_size_init();
>      socket_init();
>  
> +    if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func,
> +                          NULL, NULL)) {
> +        exit(1);
> +    }
> +

I think this should be done as early as possible.  However, we already
do -device help nearby.  Not fair to ask you to clean up this mess.

>      if (qemu_opts_foreach(qemu_find_opts("object"),
>                            user_creatable_add_opts_foreach,
>                            object_create_initial, NULL)) {