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

[Lin Ma]

>>> "Daniel P. Berrange" <address@hidden> 2016/9/12 星期一 下午 11:56 >>>
>On Sun, Sep 11, 2016 at 01:53:01PM +0800, Lin Ma wrote:
>> '-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;
>> +
>> +    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;
>
>Ewww, this struct is declared privately in object.c and
>you're declaring it here so you can poke at private
>data belonging to the object internal impl. This is
>not ok in any way.
>
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?

>> +    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)) {
>> +        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)) {
>> +                        printf("%s\n", name);
>> +                }
>> +                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));
>> +        entry->value = g_malloc0(sizeof(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")) {
>
>It is absolutely *not* safe to assume that the result of
>this condition is an enum.
>
Yes, It does not make sense and is not safe. Writing this way becasue
I can't figure out a way to judge whether the type is an enum or not.
May I have your ideas?

>> +                while (enumprop->strings[i] != NULL) {
>> +                        if (i != 0) {
>> +                                values = g_strdup_printf("%s/%s",
>> +                                                                            
>>     values, enumprop->strings[i]);
>
>Leaking the old memory for 'values'.
>
Ok, I'll fix it.
>> +                        } else {
>> +                                values = g_strdup_printf("%s", 
>> enumprop->strings[i]);
>> +                        }
>> +                        i++;
>> +                }
>> +                entry->value->type = g_strdup_printf("%s, available values: 
>> %s",
>> +                                                                            
>>             prop->type, values);
>> +        } else {
>> +                entry->value->type = g_strdup(prop->type);
>> +        }
>> +    }
>> +
>> +    start = props;
>> +    while (props != NULL) {
>> +        ObjectPropertyInfo *value = props->value;
>> +        printf("%s (%s)\n", value->name, value->type);
>> +        props = props->next;
>> +    }
>> +    qapi_free_ObjectPropertyInfoList(start);
>> +
>> +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;
>> +}
>> +
>
>> 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);
>> +    }
>
>Generally 'help' is dealt with in the main option parsing, rather than
>adding a second iteration over the options. IOW, it would normally be
>done in user_creatable_add_opts_foreach, avoiding duplicating code
>between user_creatable_add_opts_foreach and user_creatable_help_func.
>
I used to consider to put it in user_creatable_add_opts_foreach, but I think
that keeping it as a separate function maybe more clear. 
Meanwhile, I'd like to add these help output to monitor command 'object_add'
later, So puting the code into user_creatable_help_func may help as well.

>> +
>>        if (qemu_opts_foreach(qemu_find_opts("object"),
>>                                                  
>> user_creatable_add_opts_foreach,
>>                                                  object_create_initial, 
>> NULL)) {
>
>Regards,
>Daniel
 
Thanks for helping to review the code.
Lin