From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48276) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjTND-0008LG-Iu for qemu-devel@nongnu.org; Mon, 12 Sep 2016 11:42:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjTN8-0004Jb-FG for qemu-devel@nongnu.org; Mon, 12 Sep 2016 11:42:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9772) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjTN8-0004JT-7W for qemu-devel@nongnu.org; Mon, 12 Sep 2016 11:42:14 -0400 From: Markus Armbruster References: <20160911055301.26650-1-lma@suse.com> Date: Mon, 12 Sep 2016 17:42:11 +0200 In-Reply-To: <20160911055301.26650-1-lma@suse.com> (Lin Ma's message of "Sun, 11 Sep 2016 13:53:01 +0800") Message-ID: <87zindrup8.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2] object: Add 'help' option for all available backends and properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Lin Ma Cc: qemu-devel@nongnu.org, pbonzini@redhat.com Lin Ma writes: > '-object help' prints available user creatable backends. > '-object $typename,help' prints relevant properties. > > Signed-off-by: Lin Ma > --- > 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/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); > + > #endif > diff --git a/qemu-options.hx b/qemu-options.hx > index a71aaf8..fade53c 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3752,7 +3752,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", > QEMU_ARCH_ALL) > STEXI > @item -object @var{typename}[,@var{prop1}=@var{value1},...] > @@ -3762,6 +3764,9 @@ in the order they are specified. Note that the 'id' > property must be set. These objects are placed in the > '/objects' path. > > +Use '-object help' to print available backend types and > +'-object typename,help' to print relevant properties. > + Make that +Use @code{-object help} to print available backend types and +@code{-object @var{typename},help} to print relevant properties. + > @table @option > > @item -object memory-backend-file,id=@var{id},size=@var{size},mem-path=@var{dir},share=@var{on|off} > 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? > + 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. > + > + 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)) { > + 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? > + 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) { ... } > + 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. > + entry->value = g_malloc0(sizeof(ObjectPropertyInfo)); Either g_malloc0(sizeof(entry->value), or g_new0(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")) { 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? > + } 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? > + > +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. > if (qemu_opts_foreach(qemu_find_opts("object"), > user_creatable_add_opts_foreach, > object_create_initial, NULL)) {