[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all avail
From: |
Lin Ma |
Subject: |
[Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties |
Date: |
Sat, 17 Sep 2016 06:15:24 -0600 |
>>> Markus Armbruster <address@hidden> 2016/9/12 星期一 下午 11:42 >>>
>Lin Ma <address@hidden> writes:
>
>> '-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/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}[,@address@hidden,...]
>> @@ -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
> address@hidden @var{typename},help} to print relevant properties.
> +
>
Ok.
>> @table @option
>>
>> @item -object
>> memory-backend-file,address@hidden,address@hidden,address@hidden,address@hidden|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?
>
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?
>> +
>> +
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.
>> + 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?
>> +
} 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();
>> if (qemu_opts_foreach(qemu_find_opts("object"),
>>
>> user_creatable_add_opts_foreach,
>> object_create_initial,
>> NULL)) {
Thank you for reviewing the code.
Lin
- [Qemu-devel] [PATCH v2] object: Add 'help' option for all available backends and properties, Lin Ma, 2016/09/11
- Re: [Qemu-devel] [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/12
- [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties,
Lin Ma <=
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/19
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Andreas Färber, 2016/09/19
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/19
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Lin Ma, 2016/09/21
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/22
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Daniel P. Berrange, 2016/09/22
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/22
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Daniel P. Berrange, 2016/09/22
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Markus Armbruster, 2016/09/22
- Re: [Qemu-devel] 答复: Re: [PATCH v2] object: Add 'help' option for all available backends and properties, Daniel P. Berrange, 2016/09/22