Re: [Qemu-devel] [PATCH 08/12] qapi: support nested structs in OptsVisitor

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

Adding qapi maintainers to Cc, full for them quote below, please review.


For your next patch submission:
There is a "MAINTAINERS" file with the people listed.

There is a scripts/get_maintainer.pl scripts which will do the lookup
for you (accepts patch as input, prints maintainers).

You can hook this into your .git/config this way ...

  [sendemail]
        to = qemu-devel@nongnu.org
        cccmd = scripts/get_maintainer.pl --nogit-fallback

... and have 'git send-email' Cc the relevant maintainers automatically.

cheers,
  Gerd

On Fr, 2015-06-12 at 14:33 +0200, 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 the following changes:
> 1) Specifying just the field name will apply to all fields that has the
>    specified name (this means it would set both in's and out's frequency to
>    44100 in the above example).
> 2) Optionally user can specify the path in the hierarchy. Names are separated by
>    a dot (e.g. `in.frequency', `foo.bar.something', etc). The user need not
>    specify the whole path, only the last few components (i.e. `bar.something' is
>    equivalent to `foo.bar.something' if only `foo' has a `bar' field). This way
>    1) is just a special case of this when only the last component is specified.
> 3) In case of an ambiguity (e.g `frequency=44100,in.frequency=8000') the longest
>    matching (the most specific) path wins (so in this example, in's frequency
>    would become 8000, because `in.frequency' is more specific that `frequency',
>    and out's frequency would become 44100, because only `frequency' matches it).
> 
> Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com>
> ---
>  qapi/opts-visitor.c                     | 144 +++++++++++++++++++++++++-------
>  tests/qapi-schema/qapi-schema-test.json |   9 +-
>  tests/test-opts-visitor.c               |  34 ++++++++
>  3 files changed, 157 insertions(+), 30 deletions(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index f2ad6d7..409d8b7 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -64,13 +64,14 @@ struct OptsVisitor
>      /* Non-null iff depth is positive. Each key is a QemuOpt name. Each value
>       * is a non-empty GQueue, enumerating all QemuOpt occurrences with that
>       * name. */
> -    GHashTable *unprocessed_opts;
> +    GHashTable *unprocessed_opts, *opts;
>  
>      /* The list currently being traversed with opts_start_list() /
>       * opts_next_list(). The list must have a struct element type in the
>       * 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;
>  };
>  
> 
> @@ -97,11 +101,12 @@ destroy_list(gpointer list)
>  
> 
>  static void
> -opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
> +opts_visitor_insert(OptsVisitor *ov, const QemuOpt *opt)
>  {
>      GQueue *list;
> +    assert(opt);
>  
> -    list = g_hash_table_lookup(unprocessed_opts, opt->name);
> +    list = g_hash_table_lookup(ov->opts, opt->name);
>      if (list == NULL) {
>          list = g_queue_new();
>  
> @@ -109,7 +114,8 @@ opts_visitor_insert(GHashTable *unprocessed_opts, const QemuOpt *opt)
>           * "key_destroy_func" in opts_start_struct(). Thus cast away key
>           * const-ness in order to suppress gcc's warning.
>           */
> -        g_hash_table_insert(unprocessed_opts, (gpointer)opt->name, list);
> +        g_hash_table_insert(ov->opts, (gpointer)opt->name, list);
> +        g_hash_table_insert(ov->unprocessed_opts, (gpointer)opt->name, list);
>      }
>  
>      /* Similarly, destroy_list() doesn't call g_queue_free_full(). */
> @@ -127,17 +133,27 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
>      if (obj) {
>          *obj = g_malloc0(size > 0 ? size : 1);
>      }
> +
> +    /* assuming name is a statically allocated string (or at least it's lifetime
> +     * is longer than the visitor's) */
> +    if (!name) {
> +        name = "";
> +    }
> +    g_queue_push_tail(ov->nested_names, (gpointer) name);
> +
>      if (ov->depth++ > 0) {
>          return;
>      }
>  
> -    ov->unprocessed_opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> -                                                 NULL, &destroy_list);
> +    ov->opts = g_hash_table_new_full(&g_str_hash, &g_str_equal,
> +                                     NULL, &destroy_list);
> +    ov->unprocessed_opts = g_hash_table_new(&g_str_hash, &g_str_equal);
> +
>      QTAILQ_FOREACH(opt, &ov->opts_root->head, next) {
>          /* ensured by qemu-option.c::opts_do_parse() */
>          assert(strcmp(opt->name, "id") != 0);
>  
> -        opts_visitor_insert(ov->unprocessed_opts, opt);
> +        opts_visitor_insert(ov, opt);
>      }
>  
>      if (ov->opts_root->id != NULL) {
> @@ -145,7 +161,7 @@ opts_start_struct(Visitor *v, void **obj, const char *kind,
>  
>          ov->fake_id_opt->name = g_strdup("id");
>          ov->fake_id_opt->str = g_strdup(ov->opts_root->id);
> -        opts_visitor_insert(ov->unprocessed_opts, ov->fake_id_opt);
> +        opts_visitor_insert(ov, ov->fake_id_opt);
>      }
>  }
>  
> @@ -163,6 +179,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;
>      }
> @@ -177,6 +195,8 @@ opts_end_struct(Visitor *v, Error **errp)
>      }
>      g_hash_table_destroy(ov->unprocessed_opts);
>      ov->unprocessed_opts = NULL;
> +    g_hash_table_destroy(ov->opts);
> +    ov->opts = NULL;
>      if (ov->fake_id_opt) {
>          g_free(ov->fake_id_opt->name);
>          g_free(ov->fake_id_opt->str);
> @@ -185,16 +205,56 @@ opts_end_struct(Visitor *v, Error **errp)
>      ov->fake_id_opt = NULL;
>  }
>  
> +static void
> +sum_strlen(gpointer data, gpointer user_data)
> +{
> +    const char *str = data;
> +    size_t *sum_len = user_data;
>  
> +    *sum_len += strlen(str) + 1;
> +}
> +
> +static void
> +append_str(gpointer data, gpointer user_data)
> +{
> +    strcat(user_data, data);
> +    strcat(user_data, ".");
> +}
> +
> +/* lookup a name, trying from the most qualified version (e.g. foo.bar.asd) to
> + * least qualified ones (i.e. foo.bar.asd overrides bar.asd or asd) */
>  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, *key2;
> +    size_t sum_len = strlen(name);
>  
> -    list = g_hash_table_lookup(ov->unprocessed_opts, 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);
> +
> +    key2 = key;
> +    while (*key2) {
> +        list = g_hash_table_lookup(ov->opts, key2);
> +        if (list) {
> +            if (out_key) {
> +                *out_key = g_strdup(key2);
> +            }
> +            break;
> +        }
> +
> +        while (*key2 && *key2++ != '.') {
> +        }
> +    }
>      if (!list) {
>          error_set(errp, QERR_MISSING_PARAMETER, name);
>      }
> +
> +    g_free(key);
>      return list;
>  }
>  
> @@ -206,7 +266,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;
>      }
> @@ -242,11 +302,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;
> @@ -272,22 +330,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);
>  }
>  
> @@ -309,13 +373,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);
>  }
>  
> 
> @@ -325,8 +391,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;
>      }
> @@ -343,13 +410,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);
>  }
>  
> 
> @@ -361,13 +430,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;
>      }
> @@ -381,11 +451,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);
> @@ -406,6 +478,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);
>  }
>  
> 
> @@ -417,13 +490,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;
>      }
> @@ -435,11 +509,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 &&
> @@ -458,6 +534,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);
>  }
>  
> 
> @@ -468,8 +545,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;
>      }
> @@ -479,11 +557,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);
>  }
>  
> 
> @@ -494,7 +574,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);
>  }
>  
> 
> @@ -505,6 +585,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;
>  
> @@ -545,6 +627,10 @@ opts_visitor_cleanup(OptsVisitor *ov)
>      if (ov->unprocessed_opts != NULL) {
>          g_hash_table_destroy(ov->unprocessed_opts);
>      }
> +    if (ov->opts != NULL) {
> +        g_hash_table_destroy(ov->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..5862c7c 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 == 13);
> +}
> +
> +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_both, "nint=13");
> +    add_test("/visitor/opts/nested/both",        &expect_both,
> +             "sub0.nint=13,sub1.nint=13");
> +    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;
>  }