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

[Markus Armbruster <armbru@redhat.com>]

This is about QOM use.  Cc: Andreas in case he has smart ideas.
Andreas, you may want to skip ahead to "EnumProperty".

"Lin Ma" <lma@suse.com> writes:

>>>> Markus Armbruster <armbru@redhat.com> 2016/9/12 星期一 下午 11:42 >>>
>>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/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 observe
'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.

>>> +
> 	    } 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?
>>
> I agreed. will merge 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.
>>
> How about move it to here?
> 	 object_property_add_child(object_get_root(), "machine",
> 							   OBJECT(current_machine), &error_abort);
> +
> +    if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func,
> +						  NULL, NULL)) {
> +	    exit(1);
> +    }
> +
> 	 cpu_exec_init_all();

I wouldn't put it in the middle of the machine stuff.  Perhaps before
current_machine = MACHINE(...)?

>>>	  if (qemu_opts_foreach(qemu_find_opts("object"),
>>>						    user_creatable_add_opts_foreach,
>>>						    object_create_initial, NULL)) {
>
> Thank you for reviewing the code.
> Lin