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

[Lin Ma]

>>> Markus Armbruster <address@hidden> 2016/11/4 星期五 上午 3:50 >>>
>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
ok, will do.

>>  #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
ok, will do.
 
>>        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}.
ok, will do.

>>  @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-typ
e");
>> +    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.
using error_printf() instead of it, no problem.
But the current code doesn't support 'object_add help', Should I modify code to
support it in this patch?
 
>> +        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) {
Perhaps this one is better.

>> +                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()
ok, will fix them.

>> +    }
>
>There's a very similar loop in machine_help_func().  Can we avoid the
>duplication?
I would say negative. using same code snippet in machine_help_func() and
in here, we lost flexibility, says if we want to change the output 
content/format
in the future.

>> +
>> +out:
>
>You don't actually need this label.
ok, will fix it.

>> +    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()?
Nothing special, just want to let it run as early as possible. and it can't be 
run too early
because some basic structures/objects of qemu need to be constructed first.
May I have your idea?
 
 
Thanks,
Lin