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

["Daniel P. Berrange" <berrange@redhat.com>]

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 <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;
> +
> +    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.

> +    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.

> +            while (enumprop->strings[i] != NULL) {
> +                if (i != 0) {
> +                    values = g_strdup_printf("%s/%s",
> +                                            values, enumprop->strings[i]);

Leaking the old memory for 'values'.

> +                } 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.

> +
>      if (qemu_opts_foreach(qemu_find_opts("object"),
>                            user_creatable_add_opts_foreach,
>                            object_create_initial, NULL)) {

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|