[Qemu-devel] [RFC 0/6] OptsVisitor: support / flatten integer ranges for repeating options

[Qemu-devel] [RFC 0/6] OptsVisitor: support / flatten integer ranges for repeating options

[Laszlo Ersek]

Consider the following QAPI schema fragment, for the purpose of command
line parsing with OptsVisitor:

  { 'type': 'UInt16', 'data': { 'u16': 'uint16' }}

  { 'union': 'NumaOptions',
    'data': {
      'node': 'NumaNodeOptions',
      'mem' : 'NumaMemOptions' }}

  { 'type': 'NumaNodeOptions',
    'data': {
      '*nodeid': 'int',
      '*cpus'  : ['UInt16'] }}

  { 'type': 'NumaMemOptions',
    'data': {
      '*nodeid': 'int',
      '*size'  : 'size' }}

OptsVisitor already accepts the following command line with the above:

  -numa node,nodeid=3,cpus=0,cpus=1,cpus=2,cpus=6,cpus=7,cpus=8

Paolo suggested in
<http://thread.gmane.org/gmane.comp.emulators.qemu/222589/focus=222732>
that OptsVisitor should allow the following shortcut:

  -numa node,nodeid=3,cpus=0-2,cpus=6-8

and that the code processing the "cpus" list should encounter all six
elements (0, 1, 2, 6, 7, 8) individually.

The series tries to implement this. Both signed and unsigned values and
intervals should be supported:

  * 0     (zero)
  * 1-5   (one to five)
  * 4-4   (four to four, range with one element)
  * -2    (minus two)
  * -5-8  (minus two to plus eight)
  * -9--6 (minus nine to minus six)

As documented in commit eb7ee2cb, for parsing repeated options in
general, the list element type must be a struct with one mandatory
scalar field. For interval flattening, this underlying scalar type must
be an integer type. The restrictions imposed by its signedness and size
(in the above example, 'uint16') should be enforced entry-wise as usual.

That is, examining the 'uint16' example above with

  -numa node,nodeid=3,cpus=65534-65537

this is equivalent to

  -numa node,nodeid=3,cpus=65534,cpus=65535,cpus=65536,cpus=65537

and visit_type_uint16() [qapi/qapi-visit-core.c] will catch the first
element (= 65536) that has been parsed by opts_type_int() but cannot be
represented as 'uint16'.


I wrote this last night -- it is untested. I'm asking for help with a
gtester-based unit test. I'd appreciate if someone could whip up the
scaffolding (I don't even remember how to build & invoke the testers!)
and I'd fill in the test cases.

Thanks.

Laszlo Ersek (6):
  OptsVisitor: introduce basic list modes
  OptsVisitor: introduce list modes for interval flattening
  OptsVisitor: opts_type_int(): recognize intervals when LM_IN_PROGRESS
  OptsVisitor: rebase opts_type_uint64() to parse_uint_full()
  OptsVisitor: opts_type_uint64(): recognize intervals when
    LM_IN_PROGRESS
  OptsVisitor: don't try to flatten overlong integer ranges

 include/qapi/opts-visitor.h |    6 ++
 qapi/opts-visitor.c         |  159 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 135 insertions(+), 30 deletions(-)

[Qemu-devel] [RFC 1/6] OptsVisitor: introduce basic list modes

[Laszlo Ersek]

We're going to need more state while processing a list of repeated
options. This change eliminates "repeated_opts_first" and adds a new state
variable:

  list_mode       repeated_opts  repeated_opts_first
  --------------  -------------  -------------------
  LM_NONE         NULL           false
  LM_STARTED      non-NULL       true
  LM_IN_PROGRESS  non-NULL       false

Additionally, it is documented that lookup_scalar() and processed(), both
called by opts_type_XXX(), are invalid in LM_STARTED -- generated qapi
code calls opts_next_list() to allocate the very first link before trying
to parse a scalar into it. List mode restrictions are expressed in
positive / inclusive form.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi/opts-visitor.c |   43 ++++++++++++++++++++++++++++++++++---------
 1 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 174bd8b..ab9a602 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -18,6 +18,15 @@
 #include "qapi/visitor-impl.h"
 
 
+enum ListMode
+{
+    LM_NONE,             /* not traversing a list of repeated options */
+    LM_STARTED,          /* opts_start_list() succeeded */
+    LM_IN_PROGRESS       /* opts_next_list() has been called */
+};
+
+typedef enum ListMode ListMode;
+
 struct OptsVisitor
 {
     Visitor visitor;
@@ -35,8 +44,8 @@ struct OptsVisitor
     /* 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;
-    bool repeated_opts_first;
 
     /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
      * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
@@ -156,9 +165,11 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
 
     /* we can't traverse a list in a list */
-    assert(ov->repeated_opts == NULL);
+    assert(ov->list_mode == LM_NONE);
     ov->repeated_opts = lookup_distinct(ov, name, errp);
-    ov->repeated_opts_first = (ov->repeated_opts != NULL);
+    if (ov->repeated_opts != NULL) {
+        ov->list_mode = LM_STARTED;
+    }
 }
 
 
@@ -168,10 +179,13 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     GenericList **link;
 
-    if (ov->repeated_opts_first) {
-        ov->repeated_opts_first = false;
+    switch (ov->list_mode) {
+    case LM_STARTED:
+        ov->list_mode = LM_IN_PROGRESS;
         link = list;
-    } else {
+        break;
+
+    case LM_IN_PROGRESS: {
         const QemuOpt *opt;
 
         opt = g_queue_pop_head(ov->repeated_opts);
@@ -180,6 +194,11 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
             return NULL;
         }
         link = &(*list)->next;
+        break;
+    }
+
+    default:
+        assert(0);
     }
 
     *link = g_malloc0(sizeof **link);
@@ -192,14 +211,16 @@ opts_end_list(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
 
+    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
     ov->repeated_opts = NULL;
+    ov->list_mode = LM_NONE;
 }
 
 
 static const QemuOpt *
 lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
 {
-    if (ov->repeated_opts == NULL) {
+    if (ov->list_mode == LM_NONE) {
         GQueue *list;
 
         /* the last occurrence of any QemuOpt takes effect when queried by name
@@ -207,6 +228,7 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
         list = lookup_distinct(ov, name, errp);
         return list ? g_queue_peek_tail(list) : NULL;
     }
+    assert(ov->list_mode == LM_IN_PROGRESS);
     return g_queue_peek_head(ov->repeated_opts);
 }
 
@@ -214,9 +236,12 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
 static void
 processed(OptsVisitor *ov, const char *name)
 {
-    if (ov->repeated_opts == NULL) {
+    if (ov->list_mode == LM_NONE) {
         g_hash_table_remove(ov->unprocessed_opts, name);
+        return;
     }
+    assert(ov->list_mode == LM_IN_PROGRESS);
+    /* do nothing */
 }
 
 
@@ -365,7 +390,7 @@ opts_start_optional(Visitor *v, bool *present, const char *name,
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
 
     /* we only support a single mandatory scalar field in a list node */
-    assert(ov->repeated_opts == NULL);
+    assert(ov->list_mode == LM_NONE);
     *present = (lookup_distinct(ov, name, NULL) != NULL);
 }
 
-- 
1.7.1

[Qemu-devel] [RFC 2/6] OptsVisitor: introduce list modes for interval flattening

[Laszlo Ersek]

The new modes are equal-rank, exclusive sub-modes of LM_IN_PROGRESS. Teach
opts_next_list(), opts_type_int() and opts_type_uint64() to handle them.

Also enumerate explicitly what functions are valid to call in what modes:
- opts_next_list() is valid to call while flattening a range,
- opts_end_list(): ditto,
- lookup_scalar() is invalid to call during flattening; generated qapi
  traversal code must continue asking for the same kind of signed/unsigned
  list element until the interval is fully flattened,
- processed(): ditto.

List mode restrictions are always formulated in positive / inclusive
sense. The restrictions for lookup_scalar() and processed() are
automatically satisfied by current qapi traversals if the schema to build
is compatible with OptsVisitor.

The new list modes are not entered yet.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi/opts-visitor.c |   44 ++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ab9a602..e8e317f 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -22,7 +22,9 @@ enum ListMode
 {
     LM_NONE,             /* not traversing a list of repeated options */
     LM_STARTED,          /* opts_start_list() succeeded */
-    LM_IN_PROGRESS       /* opts_next_list() has been called */
+    LM_IN_PROGRESS,      /* opts_next_list() has been called */
+    LM_SIGNED_INTERVAL,  /* flattening an interval with signed bounds */
+    LM_UNSIGNED_INTERVAL /* flattening an interval with unsigned bounds */
 };
 
 typedef enum ListMode ListMode;
@@ -47,6 +49,15 @@ struct OptsVisitor
     ListMode list_mode;
     GQueue *repeated_opts;
 
+    /* When parsing a list of repeating options as integers, values of the form
+     * "a-b", representing a closed interval, are allowed. Elements in the
+     * range are generated individually.
+     */
+    union {
+        int64_t s;
+        uint64_t u;
+    } range_next, range_limit;
+
     /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
      * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
      * not survive or escape the OptsVisitor object.
@@ -185,6 +196,22 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
         link = list;
         break;
 
+    case LM_SIGNED_INTERVAL:
+    case LM_UNSIGNED_INTERVAL:
+        link = &(*list)->next;
+
+        if (ov->list_mode == LM_SIGNED_INTERVAL) {
+            if (ov->range_next.s < ov->range_limit.s) {
+                ++ov->range_next.s;
+                break;
+            }
+        } else if (ov->range_next.u < ov->range_limit.u) {
+            ++ov->range_next.u;
+            break;
+        }
+        ov->list_mode = LM_IN_PROGRESS;
+        /* range has been completed, fall through in order to pop option */
+
     case LM_IN_PROGRESS: {
         const QemuOpt *opt;
 
@@ -211,7 +238,10 @@ opts_end_list(Visitor *v, Error **errp)
 {
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
 
-    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
+    assert(ov->list_mode == LM_STARTED ||
+           ov->list_mode == LM_IN_PROGRESS ||
+           ov->list_mode == LM_SIGNED_INTERVAL ||
+           ov->list_mode == LM_UNSIGNED_INTERVAL);
     ov->repeated_opts = NULL;
     ov->list_mode = LM_NONE;
 }
@@ -303,6 +333,11 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     long long val;
     char *endptr;
 
+    if (ov->list_mode == LM_SIGNED_INTERVAL) {
+        *obj = ov->range_next.s;
+        return;
+    }
+
     opt = lookup_scalar(ov, name, errp);
     if (!opt) {
         return;
@@ -328,6 +363,11 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     const QemuOpt *opt;
     const char *str;
 
+    if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
+        *obj = ov->range_next.u;
+        return;
+    }
+
     opt = lookup_scalar(ov, name, errp);
     if (!opt) {
         return;
-- 
1.7.1

[Qemu-devel] [RFC 3/6] OptsVisitor: opts_type_int(): recognize intervals when LM_IN_PROGRESS

[Laszlo Ersek]

When a well-formed range value, bounded by signed integers, is encountered
while processing a repeated option, enter LM_SIGNED_INTERVAL and return
the low bound.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi/opts-visitor.c |   34 ++++++++++++++++++++++++++++------
 1 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index e8e317f..287aa06 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -344,15 +344,37 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
     }
     str = opt->str ? opt->str : "";
 
+    /* we've gotten past lookup_scalar() */
+    assert(ov->list_mode == LM_NONE || ov->list_mode == LM_IN_PROGRESS);
+
     errno = 0;
     val = strtoll(str, &endptr, 0);
-    if (*str != '\0' && *endptr == '\0' && errno == 0 && INT64_MIN <= val &&
-        val <= INT64_MAX) {
-        *obj = val;
-        processed(ov, name);
-        return;
+    if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
+        if (*endptr == '\0') {
+            *obj = val;
+            processed(ov, name);
+            return;
+        }
+        if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
+            long long val2;
+
+            str = endptr + 1;
+            val2 = strtoll(str, &endptr, 0);
+            if (errno == 0 && endptr > str && *endptr == '\0' &&
+                INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2) {
+                ov->range_next.s = val;
+                ov->range_limit.s = val2;
+                ov->list_mode = LM_SIGNED_INTERVAL;
+
+                /* as if entering on the top */
+                *obj = ov->range_next.s;
+                return;
+            }
+        }
     }
-    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name, "an int64 value");
+    error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
+              (ov->list_mode == LM_NONE) ? "an int64 value" :
+                                           "an int64 value or range");
 }
 
 
-- 
1.7.1

[Qemu-devel] [RFC 4/6] OptsVisitor: rebase opts_type_uint64() to parse_uint_full()

[Laszlo Ersek]

Simplify the code in preparation for the next patch.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi/opts-visitor.c |   23 +++++------------------
 1 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 287aa06..ce6c290 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -384,6 +384,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
     const QemuOpt *opt;
     const char *str;
+    unsigned long long val;
 
     if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
         *obj = ov->range_next.u;
@@ -394,26 +395,12 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     if (!opt) {
         return;
     }
-
     str = opt->str;
-    if (str != NULL) {
-        while (isspace((unsigned char)*str)) {
-            ++str;
-        }
-
-        if (*str != '-' && *str != '\0') {
-            unsigned long long val;
-            char *endptr;
 
-            /* non-empty, non-negative subject sequence */
-            errno = 0;
-            val = strtoull(str, &endptr, 0);
-            if (*endptr == '\0' && errno == 0 && val <= UINT64_MAX) {
-                *obj = val;
-                processed(ov, name);
-                return;
-            }
-        }
+    if (parse_uint_full(str, &val, 0) == 0 && val <= UINT64_MAX) {
+        *obj = val;
+        processed(ov, name);
+        return;
     }
     error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
               "an uint64 value");
-- 
1.7.1

[Qemu-devel] [RFC 5/6] OptsVisitor: opts_type_uint64(): recognize intervals when LM_IN_PROGRESS

[Laszlo Ersek]

When a well-formed range value, bounded by unsigned integers, is
encountered while processing a repeated option, enter LM_UNSIGNED_INTERVAL
and return the low bound.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 qapi/opts-visitor.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ce6c290..4413400 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -385,6 +385,7 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     const QemuOpt *opt;
     const char *str;
     unsigned long long val;
+    char *endptr;
 
     if (ov->list_mode == LM_UNSIGNED_INTERVAL) {
         *obj = ov->range_next.u;
@@ -397,13 +398,34 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
     }
     str = opt->str;
 
-    if (parse_uint_full(str, &val, 0) == 0 && val <= UINT64_MAX) {
-        *obj = val;
-        processed(ov, name);
-        return;
+    /* we've gotten past lookup_scalar() */
+    assert(ov->list_mode == LM_NONE || ov->list_mode == LM_IN_PROGRESS);
+
+    if (parse_uint(str, &val, &endptr, 0) == 0 && val <= UINT64_MAX) {
+        if (*endptr == '\0') {
+            *obj = val;
+            processed(ov, name);
+            return;
+        }
+        if (*endptr == '-' && ov->list_mode == LM_IN_PROGRESS) {
+            unsigned long long val2;
+
+            str = endptr + 1;
+            if (parse_uint_full(str, &val2, 0) == 0 &&
+                val2 <= UINT64_MAX && val <= val2) {
+                ov->range_next.u = val;
+                ov->range_limit.u = val2;
+                ov->list_mode = LM_UNSIGNED_INTERVAL;
+
+                /* as if entering on the top */
+                *obj = ov->range_next.u;
+                return;
+            }
+        }
     }
     error_set(errp, QERR_INVALID_PARAMETER_VALUE, opt->name,
-              "an uint64 value");
+              (ov->list_mode == LM_NONE) ? "a uint64 value" :
+                                           "a uint64 value or range");
 }
 
 
-- 
1.7.1

[Qemu-devel] [RFC 6/6] OptsVisitor: don't try to flatten overlong integer ranges

[Laszlo Ersek]

Prevent mistyped command line options from incurring high memory and CPU
usage at startup. 64K elements in a range should be enough for everyone
(TM).

The OPTS_VISITOR_RANGE_MAX macro is public so that unit tests can
construct corner cases with it.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/qapi/opts-visitor.h |    6 ++++++
 qapi/opts-visitor.c         |    7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/qapi/opts-visitor.h b/include/qapi/opts-visitor.h
index 5939eee..fd48c14 100644
--- a/include/qapi/opts-visitor.h
+++ b/include/qapi/opts-visitor.h
@@ -16,6 +16,12 @@
 #include "qapi/visitor.h"
 #include "qemu/option.h"
 
+/* Inclusive upper bound on the size of any flattened range. This is a safety
+ * (= anti-annoyance) measure; wrong ranges should not cause long startup
+ * delays nor exhaust virtual memory.
+ */
+#define OPTS_VISITOR_RANGE_MAX 65536
+
 typedef struct OptsVisitor OptsVisitor;
 
 /* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int"
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index 4413400..f6b089f 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -361,7 +361,9 @@ opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
             str = endptr + 1;
             val2 = strtoll(str, &endptr, 0);
             if (errno == 0 && endptr > str && *endptr == '\0' &&
-                INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2) {
+                INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2 &&
+                (val > INT64_MAX - OPTS_VISITOR_RANGE_MAX ||
+                 val2 < val + OPTS_VISITOR_RANGE_MAX)) {
                 ov->range_next.s = val;
                 ov->range_limit.s = val2;
                 ov->list_mode = LM_SIGNED_INTERVAL;
@@ -412,7 +414,8 @@ opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 
             str = endptr + 1;
             if (parse_uint_full(str, &val2, 0) == 0 &&
-                val2 <= UINT64_MAX && val <= val2) {
+                val2 <= UINT64_MAX && val <= val2 &&
+                val2 - val < OPTS_VISITOR_RANGE_MAX) {
                 ov->range_next.u = val;
                 ov->range_limit.u = val2;
                 ov->list_mode = LM_UNSIGNED_INTERVAL;
-- 
1.7.1

Re: [Qemu-devel] [RFC 1/6] OptsVisitor: introduce basic list modes

[Paolo Bonzini]

Il 18/07/2013 15:59, Laszlo Ersek ha scritto:
> We're going to need more state while processing a list of repeated
> options. This change eliminates "repeated_opts_first" and adds a new state
> variable:
> 
>   list_mode       repeated_opts  repeated_opts_first
>   --------------  -------------  -------------------
>   LM_NONE         NULL           false
>   LM_STARTED      non-NULL       true
>   LM_IN_PROGRESS  non-NULL       false
> 
> Additionally, it is documented that lookup_scalar() and processed(), both
> called by opts_type_XXX(), are invalid in LM_STARTED -- generated qapi
> code calls opts_next_list() to allocate the very first link before trying
> to parse a scalar into it. List mode restrictions are expressed in
> positive / inclusive form.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qapi/opts-visitor.c |   43 ++++++++++++++++++++++++++++++++++---------
>  1 files changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 174bd8b..ab9a602 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -18,6 +18,15 @@
>  #include "qapi/visitor-impl.h"
>  
>  
> +enum ListMode
> +{
> +    LM_NONE,             /* not traversing a list of repeated options */
> +    LM_STARTED,          /* opts_start_list() succeeded */
> +    LM_IN_PROGRESS       /* opts_next_list() has been called */
> +};
> +
> +typedef enum ListMode ListMode;
> +
>  struct OptsVisitor
>  {
>      Visitor visitor;
> @@ -35,8 +44,8 @@ struct OptsVisitor
>      /* 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;
> -    bool repeated_opts_first;
>  
>      /* If "opts_root->id" is set, reinstantiate it as a fake QemuOpt for
>       * uniformity. Only its "name" and "str" fields are set. "fake_id_opt" does
> @@ -156,9 +165,11 @@ opts_start_list(Visitor *v, const char *name, Error **errp)
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
>      /* we can't traverse a list in a list */
> -    assert(ov->repeated_opts == NULL);
> +    assert(ov->list_mode == LM_NONE);
>      ov->repeated_opts = lookup_distinct(ov, name, errp);
> -    ov->repeated_opts_first = (ov->repeated_opts != NULL);
> +    if (ov->repeated_opts != NULL) {
> +        ov->list_mode = LM_STARTED;
> +    }
>  }
>  
>  
> @@ -168,10 +179,13 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>      GenericList **link;
>  
> -    if (ov->repeated_opts_first) {
> -        ov->repeated_opts_first = false;
> +    switch (ov->list_mode) {
> +    case LM_STARTED:
> +        ov->list_mode = LM_IN_PROGRESS;
>          link = list;
> -    } else {
> +        break;
> +
> +    case LM_IN_PROGRESS: {
>          const QemuOpt *opt;
>  
>          opt = g_queue_pop_head(ov->repeated_opts);
> @@ -180,6 +194,11 @@ opts_next_list(Visitor *v, GenericList **list, Error **errp)
>              return NULL;
>          }
>          link = &(*list)->next;
> +        break;
> +    }
> +
> +    default:
> +        assert(0);

"abort()" to avoid excessively punishing folks who use -DNDEBUG (and
also to ensure they do not get extra compiler warnings).

Otherwise looks good!

Paolo

>      }
>  
>      *link = g_malloc0(sizeof **link);
> @@ -192,14 +211,16 @@ opts_end_list(Visitor *v, Error **errp)
>  {
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
> +    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
>      ov->repeated_opts = NULL;
> +    ov->list_mode = LM_NONE;
>  }
>  
>  
>  static const QemuOpt *
>  lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>  {
> -    if (ov->repeated_opts == NULL) {
> +    if (ov->list_mode == LM_NONE) {
>          GQueue *list;
>  
>          /* the last occurrence of any QemuOpt takes effect when queried by name
> @@ -207,6 +228,7 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>          list = lookup_distinct(ov, name, errp);
>          return list ? g_queue_peek_tail(list) : NULL;
>      }
> +    assert(ov->list_mode == LM_IN_PROGRESS);
>      return g_queue_peek_head(ov->repeated_opts);
>  }
>  
> @@ -214,9 +236,12 @@ lookup_scalar(const OptsVisitor *ov, const char *name, Error **errp)
>  static void
>  processed(OptsVisitor *ov, const char *name)
>  {
> -    if (ov->repeated_opts == NULL) {
> +    if (ov->list_mode == LM_NONE) {
>          g_hash_table_remove(ov->unprocessed_opts, name);
> +        return;
>      }
> +    assert(ov->list_mode == LM_IN_PROGRESS);
> +    /* do nothing */
>  }
>  
>  
> @@ -365,7 +390,7 @@ opts_start_optional(Visitor *v, bool *present, const char *name,
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
>      /* we only support a single mandatory scalar field in a list node */
> -    assert(ov->repeated_opts == NULL);
> +    assert(ov->list_mode == LM_NONE);
>      *present = (lookup_distinct(ov, name, NULL) != NULL);
>  }
>  
> 

Re: [Qemu-devel] [RFC 2/6] OptsVisitor: introduce list modes for interval flattening

[Paolo Bonzini]

Il 18/07/2013 15:59, Laszlo Ersek ha scritto:
> The new modes are equal-rank, exclusive sub-modes of LM_IN_PROGRESS. Teach
> opts_next_list(), opts_type_int() and opts_type_uint64() to handle them.

Perhaps you could use a bitmap then:

     LM_NONE = 0
     LM_STARTED = 1
     LM_IN_PROGRESS = 2
     LM_SIGNED_INTERVAL = LM_IN_PROGRESS | 4
     LM_UNSIGNED_INTERVAL = LM_IN_PROGRESS | 8

I think the only change would be that this hunk:

> @@ -211,7 +238,10 @@ opts_end_list(Visitor *v, Error **errp)
>  {
>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>  
> -    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
> +    assert(ov->list_mode == LM_STARTED ||
> +           ov->list_mode == LM_IN_PROGRESS ||
> +           ov->list_mode == LM_SIGNED_INTERVAL ||
> +           ov->list_mode == LM_UNSIGNED_INTERVAL);
>      ov->repeated_opts = NULL;
>      ov->list_mode = LM_NONE;
>  }

could be changed to

	assert(ov->list_mode == LM_STARTED ||
               (ov->list_mode & LM_IN_PROGRESS));

Paolo

Re: [Qemu-devel] [RFC 1/6] OptsVisitor: introduce basic list modes

[Laszlo Ersek]

On 07/18/13 16:53, Paolo Bonzini wrote:
> Il 18/07/2013 15:59, Laszlo Ersek ha scritto:

>> +    default:
>> +        assert(0);
> 
> "abort()" to avoid excessively punishing folks who use -DNDEBUG (and
> also to ensure they do not get extra compiler warnings).
> 
> Otherwise looks good!

Yeah I hesitated between these two. Will fix.

Thanks,
Laszlo

Re: [Qemu-devel] [RFC 2/6] OptsVisitor: introduce list modes for interval flattening

[Laszlo Ersek]

On 07/18/13 16:56, Paolo Bonzini wrote:
> Il 18/07/2013 15:59, Laszlo Ersek ha scritto:
>> The new modes are equal-rank, exclusive sub-modes of LM_IN_PROGRESS. Teach
>> opts_next_list(), opts_type_int() and opts_type_uint64() to handle them.
> 
> Perhaps you could use a bitmap then:
> 
>      LM_NONE = 0
>      LM_STARTED = 1
>      LM_IN_PROGRESS = 2
>      LM_SIGNED_INTERVAL = LM_IN_PROGRESS | 4
>      LM_UNSIGNED_INTERVAL = LM_IN_PROGRESS | 8
> 
> I think the only change would be that this hunk:
> 
>> @@ -211,7 +238,10 @@ opts_end_list(Visitor *v, Error **errp)
>>  {
>>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>>  
>> -    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
>> +    assert(ov->list_mode == LM_STARTED ||
>> +           ov->list_mode == LM_IN_PROGRESS ||
>> +           ov->list_mode == LM_SIGNED_INTERVAL ||
>> +           ov->list_mode == LM_UNSIGNED_INTERVAL);
>>      ov->repeated_opts = NULL;
>>      ov->list_mode = LM_NONE;
>>  }
> 
> could be changed to
> 
> 	assert(ov->list_mode == LM_STARTED ||
>                (ov->list_mode & LM_IN_PROGRESS));

If you don't insist (please don't :)), I wouldn't like to do this.

(a) I wanted to represent and query each individual mode explicitly
(helps with code search),

(b) the "sub-mode" nature is a theoretical thing. It only applies to the
two interval modes. This small orthogonality is limited, it doesn't
cause "combinatorial explosion" (I didn't have to double the states or
such). Most importantly, I specifically wanted one state in general to
exclude any other state. Bitmaps beget thinking about the meaning of
arbitrary variations, like 1|4, 0|2 etc; I intended to prevent such
thoughts right in the type.

Thanks
Laszlo

Re: [Qemu-devel] [RFC 2/6] OptsVisitor: introduce list modes for interval flattening

[Paolo Bonzini]

Il 18/07/2013 17:57, Laszlo Ersek ha scritto:
> On 07/18/13 16:56, Paolo Bonzini wrote:
>> Il 18/07/2013 15:59, Laszlo Ersek ha scritto:
>>> The new modes are equal-rank, exclusive sub-modes of LM_IN_PROGRESS. Teach
>>> opts_next_list(), opts_type_int() and opts_type_uint64() to handle them.
>>
>> Perhaps you could use a bitmap then:
>>
>>      LM_NONE = 0
>>      LM_STARTED = 1
>>      LM_IN_PROGRESS = 2
>>      LM_SIGNED_INTERVAL = LM_IN_PROGRESS | 4
>>      LM_UNSIGNED_INTERVAL = LM_IN_PROGRESS | 8
>>
>> I think the only change would be that this hunk:
>>
>>> @@ -211,7 +238,10 @@ opts_end_list(Visitor *v, Error **errp)
>>>  {
>>>      OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
>>>  
>>> -    assert(ov->list_mode == LM_STARTED || ov->list_mode == LM_IN_PROGRESS);
>>> +    assert(ov->list_mode == LM_STARTED ||
>>> +           ov->list_mode == LM_IN_PROGRESS ||
>>> +           ov->list_mode == LM_SIGNED_INTERVAL ||
>>> +           ov->list_mode == LM_UNSIGNED_INTERVAL);
>>>      ov->repeated_opts = NULL;
>>>      ov->list_mode = LM_NONE;
>>>  }
>>
>> could be changed to
>>
>> 	assert(ov->list_mode == LM_STARTED ||
>>                (ov->list_mode & LM_IN_PROGRESS));
> 
> If you don't insist (please don't :)), I wouldn't like to do this.

No, I won't.

> (a) I wanted to represent and query each individual mode explicitly
> (helps with code search),
> 
> (b) the "sub-mode" nature is a theoretical thing. It only applies to the
> two interval modes. This small orthogonality is limited, it doesn't
> cause "combinatorial explosion" (I didn't have to double the states or
> such). Most importantly, I specifically wanted one state in general to
> exclude any other state. Bitmaps beget thinking about the meaning of
> arbitrary variations, like 1|4, 0|2 etc; I intended to prevent such
> thoughts right in the type.

Fair enough.  But please find a way to put the "sub-mode" thing in the
code too (that's the redeeming grace of bitmaps)---even better if, at
the same time, the phrasing will calm the urge to say "bitmap!".

Paolo

Re: [Qemu-devel] [RFC 2/6] OptsVisitor: introduce list modes for interval flattening

[Laszlo Ersek]

On 07/18/13 18:03, Paolo Bonzini wrote:

> Fair enough.  But please find a way to put the "sub-mode" thing in the
> code too (that's the redeeming grace of bitmaps)---even better if, at
> the same time, the phrasing will calm the urge to say "bitmap!".

OK. I'll expand the comments on the enum constants.

Thanks!
Laszlo