qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Kővágó Zoltán
Subject: Re: [Qemu-devel] [PATCH v3 5/8] qapi: support nested structs in OptsVisitor
Date: Thu, 18 Jun 2015 19:35:44 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 
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 <address@hidden>

---

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 :)
reply via email to
[Prev in Thread] Current Thread [Next in Thread]