| Message ID | 20161020112829.14589-6-lma@suse.com |
|---|---|
| State | New |
| Headers | show |
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 | 55 +++++++++++++++++++++++++++++++++++++++++ > vl.c | 5 ++++ > 4 files changed, 68 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); > + Ugly interface: two arguments that aren't used, one of them void *. Suggest to follow the device help example: do a non-ugly interface here, and an ugly, but static wrapper in vl.c > #endif > diff --git a/qemu-options.hx b/qemu-options.hx > index b1fbdb0..b9573ce 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3761,7 +3761,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", The existing text talks about "object of type TYPENAME". The new text suddenly talks about "backend types". Switching terminology is not a good idea. Call them "object types". For comparison, here's the text we use for -device: use '-device help' to print all possible drivers use '-device driver,help' to print all possible properties Let's make this one similar: use '-object help' to print all possible object types use '-object driver,help' to print all possible properties > QEMU_ARCH_ALL) > STEXI > @item -object @var{typename}[,@var{prop1}=@var{value1},...] > @@ -3771,6 +3773,9 @@ in the order they are specified. Note that the 'id' > property must be set. These objects are placed in the > '/objects' path. > > +Use @code{-object help} to print available backend types and > +@code{-object @var{typename},help} to print relevant properties. > + -device's text: To get help on possible drivers and properties, use @code{-device help} and @code{-device @var{driver},help}. Suggest: To get help on possible object types and properties, use @code{-object help} and @code{-object @var{type},help}. > @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..da8be39 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,60 @@ void user_creatable_del(const char *id, Error **errp) > object_unparent(obj); > } > > +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp) > +{ > + const char *type = NULL; > + Object *obj = NULL; > + ObjectClass *klass; > + ObjectProperty *prop; > + ObjectPropertyIterator iter; > + > + type = qemu_opt_get(opts, "qom-type"); > + if (type && is_help_option(type)) { > + GSList *list; > + printf("Available object backend types:\n"); Wrong when running for an HMP monitor command, e.g. "object_add help". Use error_printf(). That's only slightly wrong in that it prints to stderr instead of stdout running for the command line. > + for (list = object_class_get_list(TYPE_USER_CREATABLE, false); \ > + list; \ > + list = list->next) { Superfluous line continuations. Simply write for (list = object_class_get_list(TYPE_USER_CREATABLE, false); list; list = list->next) { Alternatively, list = object_class_get_list(TYPE_USER_CREATABLE, false); for (elt = list; elt; elt = elt->next) { > + const char *name; > + name = object_class_get_name(OBJECT_CLASS(list->data)); > + if (strcmp(name, TYPE_USER_CREATABLE)) { > + printf("%s\n", name); error_printf() > + } > + } > + g_slist_free(list); Leaks the list, because by now @list is null. Suggest to fix by adopting the code under "alternatively". > + goto out; Suggest return 1, because ... > + } > + > + if (!type || !qemu_opt_has_help_opt(opts)) { > + return 0; ... you're not going to out here. No need to initialize @obj then. > + } > + > + klass = object_class_by_name(type); > + if (!klass) { > + printf("invalid object type: %s\n", type); Please use error_report(). > + goto out; return 1 would do. > + } > + if (object_class_is_abstract(klass)) { > + printf("object type '%s' is abstract\n", type); Please use error_report(). > + goto out; return 1 would do. > + } I'd put a blank line here ... > + obj = object_new(type); > + object_property_iter_init(&iter, obj); > + ... instead of here. > + while ((prop = object_property_iter_next(&iter))) { > + if (prop->description) { > + printf("%s (%s, %s)\n", prop->name, prop->type, prop->description); > + } else { > + printf("%s (%s)\n", prop->name, prop->type); > + } error_printf() > + } There's a very similar loop in machine_help_func(). Can we avoid the duplication? > + > +out: You don't actually need this label. > + object_unref(obj); > + return 1; > +} > + > static void register_types(void) > { > static const TypeInfo uc_interface_info = { > diff --git a/vl.c b/vl.c > index ebd47af..1456eca 100644 > --- a/vl.c > +++ b/vl.c > @@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp) > exit(0); > } > > + if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func, > + NULL, NULL)) { > + exit(1); > + } > + > if (!trace_init_backends()) { > exit(1); > } Can you explain why you picked this spot in main()?
>>> Markus Armbruster <armbru@redhat.com> 2016/11/4 星期五 上午 3:50 >>> >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 | 55 +++++++++++++++++++++++++++++++++++++++++ >> vl.c | 5 ++++ >> 4 files changed, 68 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); >> + > >Ugly interface: two arguments that aren't used, one of them void *. >Suggest to follow the device help example: do a non-ugly interface here, >and an ugly, but static wrapper in vl.c ok, will do. >> #endif >> diff --git a/qemu-options.hx b/qemu-options.hx >> index b1fbdb0..b9573ce 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3761,7 +3761,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", > >The existing text talks about "object of type TYPENAME". The new text >suddenly talks about "backend types". Switching terminology is not a >good idea. Call them "object types". > >For comparison, here's the text we use for -device: > > use '-device help' to print all possible drivers > use '-device driver,help' to print all possible properties > >Let's make this one similar: > > use '-object help' to print all possible object types > use '-object driver,help' to print all possible properties ok, will do. >> QEMU_ARCH_ALL) >> STEXI >> @item -object @var{typename}[,@var{prop1}=@var{value1},...] >> @@ -3771,6 +3773,9 @@ in the order they are specified. Note that the 'id' >> property must be set. These objects are placed in the >> '/objects' path. >> >> +Use @code{-object help} to print available backend types and >> +@code{-object @var{typename},help} to print relevant properties. >> + > >-device's text: > > To get help on possible drivers and properties, use @code{-device > help} and @code{-device @var{driver},help}. > >Suggest: > > To get help on possible object types and properties, use @code{-object > help} and @code{-object @var{type},help}. ok, will do. >> @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..da8be39 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,60 @@ void user_creatable_del(const char *id, Error **errp) >> object_unparent(obj); >> } >> >> +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp) >> +{ >> + const char *type = NULL; >> + Object *obj = NULL; >> + ObjectClass *klass; >> + ObjectProperty *prop; >> + ObjectPropertyIterator iter; >> + >> + type = qemu_opt_get(opts, "qom-typ e"); >> + if (type && is_help_option(type)) { >> + GSList *list; >> + printf("Available object backend types:\n"); > >Wrong when running for an HMP monitor command, e.g. "object_add help". >Use error_printf(). That's only slightly wrong in that it prints to >stderr instead of stdout running for the command line. using error_printf() instead of it, no problem. But the current code doesn't support 'object_add help', Should I modify code to support it in this patch? >> + for (list = object_class_get_list(TYPE_USER_CREATABLE, false); \ >> + list; \ >> + list = list->next) { > >Superfluous line continuations. Simply write > > for (list = object_class_get_list(TYPE_USER_CREATABLE, false); > list; > list = list->next) { > >Alternatively, > > list = object_class_get_list(TYPE_USER_CREATABLE, false); > for (elt = list; elt; elt = elt->next) { Perhaps this one is better. >> + const char *name; >> + name = object_class_get_name(OBJECT_CLASS(list->data)); >> + if (strcmp(name, TYPE_USER_CREATABLE)) { >> + printf("%s\n", name); > >error_printf() > >> + } >> + } >> + g_slist_free(list); > >Leaks the list, because by now @list is null. Suggest to fix by >adopting the code under "alternatively". > >> + goto out; > >Suggest return 1, because ... > >> + } >> + >> + if (!type || !qemu_opt_has_help_opt(opts)) { >> + return 0; > >... you're not going to out here. > >No need to initialize @obj then. > >> + } >> + >> + klass = object_class_by_name(type); >> + if (!klass) { >> + printf("invalid object type: %s\n", type); > >Please use error_report(). > >> + goto out; > >return 1 would do. > >> + } >> + if (object_class_is_abstract(klass)) { >> + printf("object type '%s' is abstract\n", type); > >Please use error_report(). > >> + goto out; > >return 1 would do. > >> + } > >I'd put a blank line here ... > >> + obj = object_new(type); >> + object_property_iter_init(&iter, obj); >> + > >... instead of here. > >> + while ((prop = object_property_iter_next(&iter))) { >> + if (prop->description) { >> + printf("%s (%s, %s)\n", prop->name, prop->type, prop->description); >> + } else { >> + printf("%s (%s)\n", prop->name, prop->type); >> + } > >error_printf() ok, will fix them. >> + } > >There's a very similar loop in machine_help_func(). Can we avoid the >duplication? I would say negative. using same code snippet in machine_help_func() and in here, we lost flexibility, says if we want to change the output content/format in the future. >> + >> +out: > >You don't actually need this label. ok, will fix it. >> + object_unref(obj); >> + return 1; >> +} >> + >> static void register_types(void) >> { >> static const TypeInfo uc_interface_info = { >> diff --git a/vl.c b/vl.c >> index ebd47af..1456eca 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp) >> exit(0); >> } >> >> + if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func, >> + NULL, NULL)) { >> + exit(1); >> + } >> + >> if (!trace_init_backends()) { >> exit(1); >> } > >Can you explain why you picked this spot in main()? Nothing special, just want to let it run as early as possible. and it can't be run too early because some basic structures/objects of qemu need to be constructed first. May I have your idea? Thanks, Lin
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 b1fbdb0..b9573ce 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3761,7 +3761,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},...] @@ -3771,6 +3773,9 @@ in the order they are specified. Note that the 'id' property must be set. These objects are placed in the '/objects' path. +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..da8be39 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,60 @@ void user_creatable_del(const char *id, Error **errp) object_unparent(obj); } +int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp) +{ + const char *type = NULL; + Object *obj = NULL; + ObjectClass *klass; + ObjectProperty *prop; + ObjectPropertyIterator iter; + + type = qemu_opt_get(opts, "qom-type"); + if (type && is_help_option(type)) { + GSList *list; + printf("Available object backend types:\n"); + for (list = object_class_get_list(TYPE_USER_CREATABLE, false); \ + list; \ + list = list->next) { + const char *name; + name = object_class_get_name(OBJECT_CLASS(list->data)); + if (strcmp(name, TYPE_USER_CREATABLE)) { + printf("%s\n", name); + } + } + g_slist_free(list); + goto out; + } + + if (!type || !qemu_opt_has_help_opt(opts)) { + return 0; + } + + klass = object_class_by_name(type); + if (!klass) { + printf("invalid object type: %s\n", type); + goto out; + } + if (object_class_is_abstract(klass)) { + printf("object type '%s' is abstract\n", type); + goto out; + } + obj = object_new(type); + object_property_iter_init(&iter, obj); + + while ((prop = object_property_iter_next(&iter))) { + if (prop->description) { + printf("%s (%s, %s)\n", prop->name, prop->type, prop->description); + } else { + printf("%s (%s)\n", prop->name, prop->type); + } + } + +out: + object_unref(obj); + return 1; +} + static void register_types(void) { static const TypeInfo uc_interface_info = { diff --git a/vl.c b/vl.c index ebd47af..1456eca 100644 --- a/vl.c +++ b/vl.c @@ -4100,6 +4100,11 @@ int main(int argc, char **argv, char **envp) exit(0); } + if (qemu_opts_foreach(qemu_find_opts("object"), user_creatable_help_func, + NULL, NULL)) { + exit(1); + } + if (!trace_init_backends()) { exit(1); }
'-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 | 55 +++++++++++++++++++++++++++++++++++++++++ vl.c | 5 ++++ 4 files changed, 68 insertions(+), 1 deletion(-)