This is about QOM use. Cc: Andreas in case he has smart ideas.
Andreas, you may want to skip ahead to "EnumProperty".
"Lin Ma" <l...@suse.com> writes:
>>>> Markus Armbruster <arm...@redhat.com> 2016/9/12 星期一 下午 11:42 >>>
>>Lin Ma <l...@suse.com> writes:
>>
>>> '-object help' prints available user creatable backends.
>>> '-object $typename,help' prints relevant properties.
>>>
>>> Signed-off-by: Lin Ma <l...@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;
>>> +
>>
>>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?
See below.
>>> +
>>> +
> 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.
Yes, please.
>>> + 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?
You're messing with struct EnumProperty because you want more help than
what ObjectPropertyInfo can provice.
Digression on the meaning of ObjectPropertyInfo. This is its
definition:
##
# @ObjectPropertyInfo:
#
# @name: the name of the property
#
# @type: the type of the property. This will typically come in one of four
# forms:
#
# 1) A primitive type such as 'u8', 'u16', 'bool', 'str', or 'double'.
# These types are mapped to the appropriate JSON type.
#
# 2) A child type in the form 'child<subtype>' where subtype is a qdev
# device type name. Child properties create the composition tree.
#
# 3) A link type in the form 'link<subtype>' where subtype is a qdev
# device type name. Link properties form the device model graph.
#
# Since: 1.2
##
{ 'struct': 'ObjectPropertyInfo',
'data': { 'name': 'str', 'type': 'str' } }
@type is documented to be either a primitive type, a child type or a
link. "Primitive type" isn't defined. The examples given suggest it's
a QAPI built-in type. If that's correct, clause 1) does not cover
enumeration types. However, it doesn't seem to be correct: I observe
'string', not 'str'. Paolo, Andreas, any idea what @type is supposed to
mean?
End of digression.
All ObjectPropertyInfo gives you is two strings: a name and a type. If
you need more information than that, you have to add a proper interface
to get it! Say a function that takes an object and a property name, and
returns information about the property's type. Probably should be two
functions, one that maps QOM object and property name to the property's
QOM type, one that maps a QOM type to QOM type information.
In other words, you need QOM object and type introspection. Paolo,
Andreas, if that already exists in some form, please point us to it.
>>> +
> } 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();
I wouldn't put it in the middle of the machine stuff. Perhaps before
current_machine = MACHINE(...)?
>>> if (qemu_opts_foreach(qemu_find_opts("object"),
>>>
>>> user_creatable_add_opts_foreach,
>>> object_create_initial,
>>> NULL)) {
>
> Thank you for reviewing the code.
> Lin
