Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor

["Kővágó Zoltán" <dirty.ice.hu@gmail.com>]

2015-06-18 19:15 keltezéssel, Laszlo Ersek írta:
> On 06/18/15 18:43, Kővágó, Zoltán wrote:
>> The current OptsVisitor flattens the whole structure, if there are same
>> named fields under different paths (like `in' and `out' in `Audiodev'),
>> the current visitor can't cope with them (for example setting
>> `frequency=44100' will set the in's frequency to 44100 and leave out's
>> frequency unspecified).
>>
>> This patch fixes it, by always requiring a complete path in case of
>> nested structs.  Fields in the path are separated by dots, similar to C
>> structs (without pointers), like `in.frequency' or`out.frequency'.
>>
>> You must provide a full path even in non-ambigous cases.  The previous
>> two commits hopefully ensures that this change doesn't create backward
>> compatibility problems.
>>
>> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
>>
>> ---
>>
>> Change from v2:
>> * only fully qualified paths are allowed
>>
>>   qapi/opts-visitor.c                     | 114 ++++++++++++++++++++++++++------
>>   tests/qapi-schema/qapi-schema-test.json |   9 ++-
>>   tests/test-opts-visitor.c               |  34 ++++++++++
>>   3 files changed, 135 insertions(+), 22 deletions(-)
>>
>> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
>> index f02059d..7a80442 100644
>> --- a/qapi/opts-visitor.c
>> +++ b/qapi/opts-visitor.c
>> @@ -71,6 +71,7 @@ struct OptsVisitor
>>        * schema, with a single mandatory scalar member. */
>>       ListMode list_mode;
>>       GQueue *repeated_opts;
>> +    char *repeated_name;
>>
>>       /* When parsing a list of repeating options as integers, values of the form
>>        * "a-b", representing a closed interval, are allowed. Elements in the
>> @@ -86,6 +87,9 @@ struct OptsVisitor
>>        * not survive or escape the OptsVisitor object.
>>        */
>>       QemuOpt *fake_id_opt;
>> +
>> +    /* List of field names leading to the current structure. */
>> +    GQueue *nested_names;
>>   };
>>
>>
>> @@ -100,6 +104,7 @@ static void
>>   opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
>>   {
>>       GQueue *list;
>> +    assert(opt);
>>
>>       list = g_hash_table_lookup(unprocessed_opts, opt->name);
>>       if (list == NULL) {
>> @@ -127,6 +132,9 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
>>       if (obj) {
>>           *obj = g_malloc0(size > 0 ? size : 1);
>>       }
>> +
>> +    g_queue_push_tail(ov->nested_names, (gpointer) name);
>> +
>>       if (ov->depth++ > 0) {
>>           return;
>>       }
>> @@ -169,6 +177,8 @@ opts_end_struct(Visitor *v, Error **errp)
>>       OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>>       GQueue *any;
>>
>> +    g_queue_pop_tail(ov->nested_names);
>> +
>>       if (--ov->depth > 0) {
>>           return;
>>       }
>> @@ -198,15 +208,54 @@ opts_end_implicit_struct(Visitor *v, Error **errp)
>>   }
>>
>>
>> +static void
>> +sum_strlen(gpointer data, gpointer user_data)
>> +{
>> +    const char *str = data;
>> +    size_t *sum_len = user_data;
>> +
>> +    if (str) { /* skip NULLs */
>> +        *sum_len += strlen(str) + 1;
>> +    }
>> +}
>> +
>> +static void
>> +append_str(gpointer data, gpointer user_data)
>> +{
>> +    const char *str = data;
>> +    char *concat_str = user_data;
>> +
>> +    if (str) {
>> +        strcat(concat_str, str);
>> +        strcat(concat_str, ".");
>> +    }
>> +}
>> +
>> +/* lookup a name, using a fully qualified version */
>>   static GQueue *
>> -lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
>> +lookup_distinct(const OptsVisitor *ov, const char *name, char **out_key,
>> +                Error **errp)
>>   {
>> -    GQueue *list;
>> +    GQueue *list = NULL;
>> +    char *key;
>> +    size_t sum_len = strlen(name);
>> +
>> +    g_queue_foreach(ov->nested_names, sum_strlen, &sum_len);
>> +    key = g_malloc(sum_len+1);
>> +    key[0] = 0;
>> +    g_queue_foreach(ov->nested_names, append_str, key);
>> +    strcat(key, name);
>> +
>> +    list = g_hash_table_lookup(ov->unprocessed_opts, key);
>> +    if (list && out_key) {
>> +        *out_key = g_strdup(key);
>> +    }
>>
>> -    list = g_hash_table_lookup(ov->unprocessed_opts, name);
>>       if (!list) {
>>           error_set(errp, QERR_MISSING_PARAMETER, name);
>>       }
>> +
>> +    g_free(key);
>>       return list;
>>   }
>>
>> @@ -218,7 +267,7 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
>>
>>       /* we can't traverse a list in a list */
>>       assert(ov->list_mode == LM_NONE);
>> -    ov->repeated_opts = lookup_distinct(ov, name, errp);
>> +    ov->repeated_opts = lookup_distinct(ov, name, &ov->repeated_name, errp);
>>       if (ov->repeated_opts != NULL) {
>>           ov->list_mode = LM_STARTED;
>>       }
>> @@ -254,11 +303,9 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
>>           /* range has been completed, fall through in order to pop option */
>>
>>       case LM_IN_PROGRESS: {
>> -        const QemuOpt *opt;
>> -
>> -        opt = g_queue_pop_head(ov->repeated_opts);
>> +        g_queue_pop_head(ov->repeated_opts);
>>           if (g_queue_is_empty(ov->repeated_opts)) {
>> -            g_hash_table_remove(ov->unprocessed_opts, opt->name);
>> +            g_hash_table_remove(ov->unprocessed_opts, ov->repeated_name);
>>               return NULL;
>>           }
>>           link = &(*list)->next;
>> @@ -284,22 +331,28 @@ opts_end_list(Visitor *v, Error **errp)
>>              ov->list_mode == LM_SIGNED_INTERVAL ||
>>              ov->list_mode == LM_UNSIGNED_INTERVAL);
>>       ov->repeated_opts = NULL;
>> +
>> +    g_free(ov->repeated_name);
>> +    ov->repeated_name = NULL;
>> +
>>       ov->list_mode = LM_NONE;
>>   }
>>
>>
>>   static const QemuOpt *
>> -lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>> +lookup_scalar(const OptsVisitor *ov, const char *name, char** out_key,
>> +              Error **errp)
>>   {
>>       if (ov->list_mode == LM_NONE) {
>>           GQueue *list;
>>
>>           /* the last occurrence of any QemuOpt takes effect when queried by name
>>            */
>> -        list = lookup_distinct(ov, name, errp);
>> +        list = lookup_distinct(ov, name, out_key, errp);
>>           return list ? g_queue_peek_tail(list) : NULL;
>>       }
>>       assert(ov->list_mode == LM_IN_PROGRESS);
>> +    assert(out_key == NULL || *out_key == NULL);
>>       return g_queue_peek_head(ov->repeated_opts);
>>   }
>>
>> @@ -321,13 +374,15 @@ opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
>>   {
>>       OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>>       const QemuOpt *opt;
>> +    char *key = NULL;
>>
>> -    opt = lookup_scalar(ov, name, errp);
>> +    opt = lookup_scalar(ov, name, &key, errp);
>>       if (!opt) {
>>           return;
>>       }
>>       *obj = g_strdup(opt->str ? opt->str : "");
>> -    processed(ov, name);
>> +    processed(ov, key);
>> +    g_free(key);
>>   }
>>
>>
>> @@ -337,8 +392,9 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
>>   {
>>       OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>>       const QemuOpt *opt;
>> +    char *key = NULL;
>>
>> -    opt = lookup_scalar(ov, name, errp);
>> +    opt = lookup_scalar(ov, name, &key, errp);
>>       if (!opt) {
>>           return;
>>       }
>> @@ -355,13 +411,15 @@ opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
>>           } else {
>>               error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>>                   "on|yes|y|off|no|n");
>> +            g_free(key);
>>               return;
>>           }
>>       } else {
>>           *obj = true;
>>       }
>>
>> -    processed(ov, name);
>> +    processed(ov, key);
>> +    g_free(key);
>>   }
>>
>>
>> @@ -373,13 +431,14 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>>       const char *str;
>>       long long val;
>>       char *endptr;
>> +    char *key = NULL;
>>
>>       if (ov->list_mode == LM_SIGNED_INTERVAL) {
>>           *obj = ov->range_next.s;
>>           return;
>>       }
>>
>> -    opt = lookup_scalar(ov, name, errp);
>> +    opt = lookup_scalar(ov, name, &key, errp);
>>       if (!opt) {
>>           return;
>>       }
>> @@ -393,11 +452,13 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>>       if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
>>           if (*endptr == '\0') {
>>               *obj = val;
>> -            processed(ov, name);
>> +            processed(ov, key);
>> +            g_free(key);
>>               return;
>>           }
>>           if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
>>               long long val2;
>> +            assert(key == NULL);
>>
>>               str = endptr + 1;
>>               val2 = strtoll(str, &endptr, 0);
>> @@ -418,6 +479,7 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
>>       error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>>                 (ov->list_mode == LM_NONE) ? "an int64 value" :
>>                                              "an int64 value or range");
>> +    g_free(key);
>>   }
>>
>>
>> @@ -429,13 +491,14 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>>       const char *str;
>>       unsigned long long val;
>>       char *endptr;
>> +    char *key = NULL;
>>
>>       if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
>>           *obj = ov->range_next.u;
>>           return;
>>       }
>>
>> -    opt = lookup_scalar(ov, name, errp);
>> +    opt = lookup_scalar(ov, name, &key, errp);
>>       if (!opt) {
>>           return;
>>       }
>> @@ -447,11 +510,13 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>>       if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) {
>>           if (*endptr == '\0') {
>>               *obj = val;
>> -            processed(ov, name);
>> +            processed(ov, key);
>> +            g_free(key);
>>               return;
>>           }
>>           if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
>>               unsigned long long val2;
>> +            assert(key == NULL);
>>
>>               str = endptr + 1;
>>               if (parse_uint_full(str, &val2, 0) == 0 &&
>> @@ -470,6 +535,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>>       error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>>                 (ov->list_mode == LM_NONE) ? "a uint64 value" :
>>                                              "a uint64 value or range");
>> +    g_free(key);
>>   }
>>
>>
>> @@ -480,8 +546,9 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>>       const QemuOpt *opt;
>>       int64_t val;
>>       char *endptr;
>> +    char *key = NULL;
>>
>> -    opt = lookup_scalar(ov, name, errp);
>> +    opt = lookup_scalar(ov, name, &key, errp);
>>       if (!opt) {
>>           return;
>>       }
>> @@ -491,11 +558,13 @@ opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
>>       if (val < 0 || *endptr) {
>>           error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
>>                     "a size value representible as a non-negative int64");
>> +        g_free(key);
>>           return;
>>       }
>>
>>       *obj = val;
>> -    processed(ov, name);
>> +    processed(ov, key);
>> +    g_free(key);
>>   }
>>
>>
>> @@ -506,7 +575,7 @@ opts_optional(Visitor *v, bool *present, const char *name, Error **errp)
>>
>>       /* we only support a single mandatory scalar field in a list node */
>>       assert(ov->list_mode == LM_NONE);
>> -    *present = (lookup_distinct(ov, name, NULL) != NULL);
>> +    *present = (lookup_distinct(ov, name, NULL, NULL) != NULL);
>>   }
>>
>>
>> @@ -517,6 +586,8 @@ opts_visitor_new(const QemuOpts *opts)
>>
>>       ov = g_malloc0(sizeof *ov);
>>
>> +    ov->nested_names = g_queue_new();
>> +
>>       ov->visitor.start_struct = &opts_start_struct;
>>       ov->visitor.end_struct   = &opts_end_struct;
>>
>> @@ -560,6 +631,7 @@ opts_visitor_cleanup(OptsVisitor *ov)
>>       if (ov->unprocessed_opts != NULL) {
>>           g_hash_table_destroy(ov->unprocessed_opts);
>>       }
>> +    g_queue_free(ov->nested_names);
>>       g_free(ov->fake_id_opt);
>>       g_free(ov);
>>   }
>> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>> index c7eaa86..a818eff 100644
>> --- a/tests/qapi-schema/qapi-schema-test.json
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -81,6 +81,11 @@
>>   { 'command': 'user_def_cmd3', 'data': {'a': 'int', '*b': 'int' },
>>     'returns': 'int' }
>>
>> +# For testing hierarchy support in opts-visitor
>> +{ 'struct': 'UserDefOptionsSub',
>> +  'data': {
>> +    '*nint': 'int' } }
>> +
>>   # For testing integer range flattening in opts-visitor. The following schema
>>   # corresponds to the option format:
>>   #
>> @@ -94,7 +99,9 @@
>>       '*u64' : [ 'uint64' ],
>>       '*u16' : [ 'uint16' ],
>>       '*i64x':   'int'     ,
>> -    '*u64x':   'uint64'  } }
>> +    '*u64x':   'uint64'  ,
>> +    'sub0':    'UserDefOptionsSub',
>> +    'sub1':    'UserDefOptionsSub' } }
>>
>>   # testing event
>>   { 'struct': 'EventStructOne',
>> diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
>> index ebeee5d..9199141 100644
>> --- a/tests/test-opts-visitor.c
>> +++ b/tests/test-opts-visitor.c
>> @@ -177,6 +177,34 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer test_data)
>>       g_assert(f->userdef->u64->value == UINT64_MAX);
>>   }
>>
>> +static void
>> +expect_both(OptsVisitorFixture *f, gconstpointer test_data)
>> +{
>> +    expect_ok(f, test_data);
>> +    g_assert(f->userdef->sub0->has_nint);
>> +    g_assert(f->userdef->sub0->nint == 13);
>> +    g_assert(f->userdef->sub1->has_nint);
>> +    g_assert(f->userdef->sub1->nint == 17);
>> +}
>> +
>> +static void
>> +expect_sub0(OptsVisitorFixture *f, gconstpointer test_data)
>> +{
>> +    expect_ok(f, test_data);
>> +    g_assert(f->userdef->sub0->has_nint);
>> +    g_assert(f->userdef->sub0->nint == 13);
>> +    g_assert(!f->userdef->sub1->has_nint);
>> +}
>> +
>> +static void
>> +expect_sub1(OptsVisitorFixture *f, gconstpointer test_data)
>> +{
>> +    expect_ok(f, test_data);
>> +    g_assert(!f->userdef->sub0->has_nint);
>> +    g_assert(f->userdef->sub1->has_nint);
>> +    g_assert(f->userdef->sub1->nint == 13);
>> +}
>> +
>>   /* test cases */
>>
>>   int
>> @@ -270,6 +298,12 @@ main(int argc, char **argv)
>>       add_test("/visitor/opts/i64/range/2big/full", &expect_fail,
>>                "i64=-0x8000000000000000-0x7fffffffffffffff");
>>
>> +    /* Test nested structs support */
>> +    add_test("/visitor/opts/nested/unqualified", &expect_fail, "nint=13");
>> +    add_test("/visitor/opts/nested/both",        &expect_both,
>> +             "sub0.nint=13,sub1.nint=17");
>> +    add_test("/visitor/opts/nested/sub0",        &expect_sub0, "sub0.nint=13");
>> +    add_test("/visitor/opts/nested/sub1",        &expect_sub1, "sub1.nint=13");
>>       g_test_run();
>>       return 0;
>>   }
>>
>
> Re patch v3 1/8 -- apparently I was not around when the
> start_implicit_struct() and end_implicit_struct() visitor callbacks were
> introduced, so I don't know what they are good for, but on the surface,
> that patch might even be correct. I can't tell.

Actually I'm not completely sure that the visitor code generated are 
correct in case of implicit structs.  In case of normal structs, there's 
an if (*obj) { ... } check, but in case of implicit structs it just 
visits the fields without checking (which is problematic, since the 
default visit_start_implicit_struct is just a no-op, causing segfaults...)

> Re patch v3 5/8, ie. this patch -- if there has been some kind of
> "master blurb" for this feature, ie. justification for nested structs,
> I'm unaware of it. (I'm not getting qemu-devel email, just personal
> Cc's.) Can you please send me a link into the archive where this feature
> is justifed / discussed?

This is needed for my -audiodev option, see v2 patch thread: 
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04189.html
And probably this as an example where it's useful:
http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg04186.html

> I have one related, and one independent note here:
>
> (1) The related note -- OptsVisitor was introduced in commit eb7ee2cbeb.
> You can see in the commit message that "union of flat structures" was a
> foundational design trait for OptsVisitor, so if you want nested
> structs, it's not a "bug to fix", but a new feature to add.
>
> (Namely, refer to the allowed members of "struct for discriminator case
> 1" -- they can only be scalar members, or restricted format lists for
> repeating optargs. Structure nesting is not allowed on any level.)
>
> The objective was to support then-existent command lines (ie. parse
> options into QAPI schemas). As far as I (vaguely) recall, Eric has since
> (independently) flattened a bunch of QAPI structs as well. So I'm unsure
> if and why nested structs are "again" a "thing".
>
> (In any case this should not be taken as my opposition; I'm just
> uninformed. That link I mentioned above could inform me.)

Check my second link, it contains a good example where it's useful (to 
specify input and output audio settings independently, like in.frequency 
and out.frequency.  Of course we could do like in-frequency and 
out-frequency and a bunch of copy paste, but this is a much better way 
imho, even if it requires some changes to how existing things work).

> (2) The independent note. OptsVisitor is one of the
> hardest-to-understand things I've implemented. It has always been hard
> to understand for myself. As far as I recall, I struggled in my mind to
> cover all corner cases and prove stuff right. Later I was "asked" to add
> range support to it (ie. turn a range like 3-7 into a list of integers
> 3, 4, 5, 6, 7) and I was sweating bloody bullets to avoid regressions.
>
> So, this code requires careful tiptoeing. I do not want to discourage
> changes to it (and I certainly encourage reviewers to look at your
> patches). I'm just saying I personally can't be your reviewer. I lost
> this context long ago, I dread looking at this code again, and I don't
> feel safe saying either yay or nay.
>
> You could *almost* accuse me of having thrown this code over the fence.
> My only excuse (a good one though) is that I only wrote this code
> because I was asked to. (If you're familiar with the Bioshock universe,
> I heard "would you kindly".)
>
> I'm told that this code has proved somewhat useful, which I'm glad
> about, but I really can't support it. Sorry about that. So, I'm neither
> acking nor nacking; don't wait for my feedback.
>
> (Apologies for the wall of text, but I guess it's better to say "sorry
> can't help you" in too many words than not reacting at all.)

All right, no problem.

>
> Thanks
> László (since we're using Hungarian accents in this thread :))
>

Thanks
Zoltán :)