Re: [Qemu-devel] [PATCH v19 07/11] qapi: make string output visitor parse int list

[Hu Tao]

On Tue, Mar 04, 2014 at 10:03:49AM +0100, Paolo Bonzini wrote:
> Il 04/03/2014 08:28, Hu Tao ha scritto:
> >From: Hu Tao <address@hidden>
> >
> >Signed-off-by: Hu Tao <address@hidden>
> >---
> > qapi/string-output-visitor.c       | 156 
> > +++++++++++++++++++++++++++++++++++--
> > tests/test-string-output-visitor.c |  26 +++++++
> > 2 files changed, 177 insertions(+), 5 deletions(-)
> >
> >diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> >index fb1d2e8..bc9bb36 100644
> >--- a/qapi/string-output-visitor.c
> >+++ b/qapi/string-output-visitor.c
> >@@ -17,11 +17,57 @@
> > #include "qemu/host-utils.h"
> > #include <math.h>
> >
> >+enum ListMode {
> >+    LM_NONE,             /* not traversing a list of repeated options */
> >+    LM_STARTED,          /* start_list() succeeded */
> >+
> >+    LM_IN_PROGRESS,      /* next_list() has been called.
> >+                          *
> >+                          * Generating the next list link will consume the 
> >most
> >+                          * recently parsed QemuOpt instance of the repeated
> >+                          * option.
> >+                          *
> >+                          * Parsing a value into the list link will examine 
> >the
> >+                          * next QemuOpt instance of the repeated option, 
> >and
> >+                          * possibly enter LM_SIGNED_INTERVAL or
> >+                          * LM_UNSIGNED_INTERVAL.
> >+                          */
> >+
> >+    LM_SIGNED_INTERVAL,  /* next_list() has been called.
> >+                          *
> >+                          * Generating the next list link will consume the 
> >most
> >+                          * recently stored element from the signed 
> >interval,
> >+                          * parsed from the most recent QemuOpt instance of 
> >the
> >+                          * repeated option. This may consume QemuOpt itself
> >+                          * and return to LM_IN_PROGRESS.
> >+                          *
> >+                          * Parsing a value into the list link will store 
> >the
> >+                          * next element of the signed interval.
> >+                          */
> >+
> >+    LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */
> >+
> >+    LM_END
> >+};
> >+
> >+typedef enum ListMode ListMode;
> >+
> > struct StringOutputVisitor
> > {
> >     Visitor visitor;
> >     bool human;
> >+    bool head;
> >     char *string;
> >+    ListMode list_mode;
> >+    /* 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_start, range_end;
> >+
> > };
> >
> > static void string_output_set(StringOutputVisitor *sov, char *string)
> >@@ -34,13 +80,60 @@ static void print_type_int(Visitor *v, int64_t *obj, 
> >const char *name,
> >                            Error **errp)
> > {
> >     StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
> >-    char *out;
> >+    char *out = NULL;
> >
> >-    if (sov->human) {
> >-        out = g_strdup_printf("%lld (%#llx)", (long long) *obj, (long long) 
> >*obj);
> >-    } else {
> >-        out = g_strdup_printf("%lld", (long long) *obj);
> >+    switch (sov->list_mode) {
> >+    case LM_NONE:
> >+        if (sov->human) {
> >+            out = g_strdup_printf("%lld (%#llx)", (long long) *obj,
> >+                                  (long long) *obj);
> >+        } else {
> >+            out = g_strdup_printf("%lld", (long long) *obj);
> >+        }
> >+        sov->list_mode = LM_END;
> >+        break;
> >+
> >+    case LM_STARTED:
> >+        sov->range_start.s = *obj;
> >+        sov->range_end.s = *obj;
> >+        sov->list_mode = LM_IN_PROGRESS;
> >+        break;
> >+
> >+    case LM_IN_PROGRESS:
> >+        assert(sov->range_end.s + 1 == *obj);
> >+        sov->range_end.s++;
> >+        break;
> >+
> >+    case LM_END:
> >+        assert(sov->range_end.s + 1 == *obj);
> >+        sov->range_end.s++;
> >+        if (sov->range_end.s == sov->range_start.s) {
> >+            if (sov->human) {
> >+                out = g_strdup_printf("%lld (%#llx)",
> >+                                      (long long)sov->range_start.s,
> >+                                      (long long)sov->range_start.s);
> >+            } else {
> >+                out = g_strdup_printf("%lld", (long 
> >long)sov->range_start.s);
> >+            }
> >+        } else {
> >+            if (sov->human) {
> >+                out = g_strdup_printf("%lld(%#llx)-%lld(%#llx)",
> >+                                      (long long) sov->range_start.s,
> >+                                      (long long) sov->range_start.s,
> >+                                      (long long) sov->range_end.s,
> >+                                      (long long) sov->range_end.s);
> 
> Perhaps "10-15 (0xa-0xf)"?

Looks better.

> 
> >+            } else {
> >+                out = g_strdup_printf("%lld-%lld",
> >+                                      (long long) sov->range_start.s,
> >+                                      (long long) sov->range_end.s);
> >+            }
> >+        }
> 
> This looks wrong.  You do not insert any separator, and you do not
> handle things like "0-3,8-11".  You probably should use a GString
> instead of string_output_set.

Right. We should also handle "0-3,8-11"-like lists in string input
visitor and opts visitor.