[08/25] qapi: support nested structs in OptsVisitor

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 qapi
flattening 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>
---
 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(-)

Hi

On Thu, Aug 6, 2015 at 8:28 PM, Kővágó, Zoltán <dirty.ice.hu@gmail.com> 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 qapi
> flattening 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>
> ---
>  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 aa68814..ff61d42 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);

Instead of using a GQueue, I think you could use a GArray, and use
g_strjoin() here.

> +
> +    list = g_hash_table_lookup(ov->unprocessed_opts, key);
> +    if (list && out_key) {
> +        *out_key = g_strdup(key);

or just *out_key = key; key = NULL; (g_free accepts NULL)

> +    }
>
> -    list = g_hash_table_lookup(ov->unprocessed_opts, name);
>      if (!list) {
>          error_setg(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);

It would make sense to add an assert(ov->repeated_name == NULL) imho,
this could catch potential leaks.

> +    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_setg(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_setg(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_setg(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_setg(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',

make check fails with this patch, you need to update
tests/qapi-schema/qapi-schema-test.out.

> +    'sub1':    'UserDefOptionsSub' } }
>
>  # testing event
>  { 'struct': 'EventStructOne',
> diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
> index 1c753d9..4393266 100644
> --- a/tests/test-opts-visitor.c
> +++ b/tests/test-opts-visitor.c
> @@ -178,6 +178,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
> @@ -271,6 +299,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;
>  }
> --
> 2.4.5
>
>

Hi,

2015-08-20 17:55 keltezéssel, Marc-André Lureau írta:
[snip]
>> +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);
>
> Instead of using a GQueue, I think you could use a GArray, and use
> g_strjoin() here.

Unfortunately g_strjoinv won't skip NULL pointers in the array (as it 
expects a NULL terminated array), but we will get structs with NULL name 
(the top level struct will be such one).  And one can't simply say if 
(name) { g_array_append_val(...); } in opts_start_struct because we need 
to know somehow in opts_end_struct if we need to pop or not.

One way is to use NULL pointers because in that case we must pop, but 
g_strjoinv won't work.  An alternate is to only push non-NULL elements 
into the array and have another bool array where we store if we need to 
pop or not.  Not sure which one is cleaner.

Or maybe if it's guaranteed that only the top-level struct will have a 
NULL name, we can hack around by only pushing non-NULL elements and 
ignore empty array when popping.

Zoltan