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

[Andreas Färber]

Hi Lin and Markus,

Am 19.09.2016 um 13:58 schrieb Markus Armbruster:
> This is about QOM use.  Cc: Andreas in case he has smart ideas.
> Andreas, you may want to skip ahead to "EnumProperty".
> 
> "Lin Ma" <address@hidden> writes:
> 
>>>>> Markus Armbruster <address@hidden> 2016/9/12 星期一 下午 11:42 >>>
>>> 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             | 112 
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>>  vl.c                                               |   5 ++
>>>>  4 files changed, 125 insertions(+), 1 deletion(-)
>>>>
> [...]
>>>> 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?
>>>
>> I'll remove it.
>>
>>>> +    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.
>>>
>> Yes, this way is ugly.
>> What I want to do is parsing the enum <-> string mappings through the 
>> EnumProperty
>> to make the output more reasonable.
>> It should be parsed in object.c, But I can't assume the size of enum str 
>> list, then can't
>> pre-alloc it in user_creatable_help_func. So far I havn't figured out a good 
>> way to return
>> a string array that including the enum str list to user_creatable_help_func 
>> for printing.
>> May I have your suggestions?
> 
> See below.
> 
>>>> +
>>>> +   
>>  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)) {
>>>
>> Yes, it is much simpler, I'll use this instead of visitor code.
>>
>>>> +      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?
>>>
>> Yes, The object type user-creatable is abstract, But obviously it missed the 
>> abstract property.
>> I'll add it in next patch(patch 1/2), Then I dont need to skip it at here 
>> anymore.
> 
> Yes, please.
> 
>>>> +                      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) {
>>>                        ...
>>>                }
>>>
>> Will do it.
>>
>>>> +      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.
>>>
>> ok.
>>
>>>> +      entry->value = g_malloc0(sizeof(ObjectPropertyInfo));
>>>
>>> Either g_malloc0(sizeof(entry->value), or g_new0(ObjectPropertyInfo).
>>>
>> will use g_malloc0(sizeof(entry->value).
>>
>>>> +      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?
>>>
>> Yes, After filter all the other types above, I assume the result here is the 
>> enum case.
>> I knew this way does not make sense, But I havn't figured out a proper way 
>> yet to judge
>> whether the type is an enum or not.
>> May I have your ideas?
> 
> You're messing with struct EnumProperty because you want more help than
> what ObjectPropertyInfo can provice.
> 
> Digression on the meaning of ObjectPropertyInfo.  This is its
> definition:
> 
> ##
> # @ObjectPropertyInfo:
> #
> # @name: the name of the property
> #
> # @type: the type of the property.  This will typically come in one of four
> #        forms:
> #
> #        1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
> #           These types are mapped to the appropriate JSON type.
> #
> #        2) A child type in the form 'child<subtype>' where subtype is a qdev
> #           device type name.  Child properties create the composition tree.
> #
> #        3) A link type in the form 'link<subtype>' where subtype is a qdev
> #           device type name.  Link properties form the device model graph.
> #
> # Since: 1.2
> ##
> { 'struct': 'ObjectPropertyInfo',
>   'data': { 'name': 'str', 'type': 'str' } }
> 
> @type is documented to be either a primitive type, a child type or a
> link.  "Primitive type" isn't defined.  The examples given suggest it's
> a QAPI built-in type.  If that's correct, clause 1) does not cover
> enumeration types.  However, it doesn't seem to be correct: I observ
> 'string', not 'str'.  Paolo, Andreas, any idea what @type is supposed to
> mean?
> 
> End of digression.
> 
> All ObjectPropertyInfo gives you is two strings: a name and a type.  If
> you need more information than that, you have to add a proper interface
> to get it!  Say a function that takes an object and a property name, and
> returns information about the property's type.  Probably should be two
> functions, one that maps QOM object and property name to the property's
> QOM type, one that maps a QOM type to QOM type information.
> 
> In other words, you need QOM object and type introspection.  Paolo,
> Andreas, if that already exists in some form, please point us to it.

Could you say what exactly you want to introspect here?

Both ObjectClass and Object have a list of properties that together form
the list of properties that can be set on an instance. So you'll need to
instantiate the object like I think we do for devices. Then you can
recursively enumerate the properties to get their names and types (and
possibly put them into a new list for alphabetical sorting if desired).

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)