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

[Markus Armbruster]

Lin Ma <address@hidden> writes:

> '-object help' prints available user creatable backends.
> '-object $typename,help' prints relevant properties.
>
> Signed-off-by: Lin Ma <address@hidden>
> ---
>  include/qom/object_interfaces.h |  2 ++
>  qemu-options.hx                 |  7 +++++-
>  qom/object_interfaces.c         | 55 
> +++++++++++++++++++++++++++++++++++++++++
>  vl.c                            |  5 ++++
>  4 files changed, 68 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);
> +

Ugly interface: two arguments that aren't used, one of them void *.
Suggest to follow the device help example: do a non-ugly interface here,
and an ugly, but static wrapper in vl.c

>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b1fbdb0..b9573ce 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3761,7 +3761,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",

The existing text talks about "object of type TYPENAME".  The new text
suddenly talks about "backend types".  Switching terminology is not a
good idea.  Call them "object types".

For comparison, here's the text we use for -device:

                use '-device help' to print all possible drivers
                use '-device driver,help' to print all possible properties

Let's make this one similar:

                use '-object help' to print all possible object types
                use '-object driver,help' to print all possible properties

>      QEMU_ARCH_ALL)
>  STEXI
>  @item -object @var{typename}[,@address@hidden,...]
> @@ -3771,6 +3773,9 @@ in the order they are specified.  Note that the 'id'
>  property must be set.  These objects are placed in the
>  '/objects' path.
>  
> +Use @code{-object help} to print available backend types and
> address@hidden @var{typename},help} to print relevant properties.
> +

-device's text:

    To get help on possible drivers and properties, use @code{-device
    help} and @code{-device @var{driver},help}.

Suggest:

    To get help on possible object types and properties, use @code{-object
    help} and @code{-object @var{type},help}.

>  @table @option
>  
>  @item -object 
> memory-backend-file,address@hidden,address@hidden,address@hidden,address@hidden|off}
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index bf59846..da8be39 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,60 @@ void user_creatable_del(const char *id, Error **errp)
>      object_unparent(obj);
>  }
>  
> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
> +{
> +    const char *type = NULL;
> +    Object *obj = NULL;
> +    ObjectClass *klass;
> +    ObjectProperty *prop;
> +    ObjectPropertyIterator iter;
> +
> +    type = qemu_opt_get(opts, "qom-type");
> +    if (type && is_help_option(type)) {
> +        GSList *list;
> +        printf("Available object backend types:\n");

Wrong when running for an HMP monitor command, e.g. "object_add help".
Use error_printf().  That's only slightly wrong in that it prints to
stderr instead of stdout running for the command line.

> +        for (list = object_class_get_list(TYPE_USER_CREATABLE, false);  \
> +                list;                                                   \
> +                list = list->next) {

Superfluous line continuations.  Simply write

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

Alternatively,

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

> +            const char *name;
> +            name = object_class_get_name(OBJECT_CLASS(list->data));
> +            if (strcmp(name, TYPE_USER_CREATABLE)) {
> +                printf("%s\n", name);

error_printf()

> +            }
> +        }
> +        g_slist_free(list);

Leaks the list, because by now @list is null.  Suggest to fix by
adopting the code under "alternatively".

> +        goto out;

Suggest return 1, because ...

> +    }
> +
> +    if (!type || !qemu_opt_has_help_opt(opts)) {
> +        return 0;

... you're not going to out here.

No need to initialize @obj then.

> +    }
> +
> +    klass = object_class_by_name(type);
> +    if (!klass) {
> +        printf("invalid object type: %s\n", type);

Please use error_report().

> +        goto out;

return 1 would do.

> +    }
> +    if (object_class_is_abstract(klass)) {
> +        printf("object type '%s' is abstract\n", type);

Please use error_report().

> +        goto out;

return 1 would do.

> +    }

I'd put a blank line here ...

> +    obj = object_new(type);
> +    object_property_iter_init(&iter, obj);
> +

... instead of here.

> +    while ((prop = object_property_iter_next(&iter))) {
> +        if (prop->description) {
> +            printf("%s (%s, %s)\n", prop->name, prop->type, 
> prop->description);
> +        } else {
> +            printf("%s (%s)\n", prop->name, prop->type);
> +        }

error_printf()

> +    }

There's a very similar loop in machine_help_func().  Can we avoid the
duplication?

> +
> +out:

You don't actually need this label.

> +    object_unref(obj);
> +    return 1;
> +}
> +
>  static void register_types(void)
>  {
>      static const TypeInfo uc_interface_info = {
> diff --git a/vl.c b/vl.c
> index ebd47af..1456eca 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> +    if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func,
> +                          NULL, NULL)) {
> +        exit(1);
> +    }
> +
>      if (!trace_init_backends()) {
>          exit(1);
>      }

Can you explain why you picked this spot in main()?