diff mbox

[PULL,091/103] qapi: make string input visitor parse int list

Message ID 1403021756-15960-92-git-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin June 17, 2014, 5:41 p.m. UTC 
From: Hu Tao <hutao@cn.fujitsu.com>

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Tested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

MST: split up patch
---
 qapi/string-input-visitor.c       | 195 ++++++++++++++++++++++++++++++++++++--
 tests/test-string-input-visitor.c |  36 +++++++
 2 files changed, 223 insertions(+), 8 deletions(-)

Comments

Eric Blake June 17, 2014, 9:36 p.m. UTC | #1 
On 06/17/2014 11:41 AM, Michael S. Tsirkin wrote:
> From: Hu Tao <hutao@cn.fujitsu.com>
> 
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Tested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> MST: split up patch
> ---
>  qapi/string-input-visitor.c       | 195 ++++++++++++++++++++++++++++++++++++--
>  tests/test-string-input-visitor.c |  36 +++++++
>  2 files changed, 223 insertions(+), 8 deletions(-)
> 

> +static void parse_str(StringInputVisitor *siv, Error **errp)
> +{
> +    char *str = (char *) siv->string;
> +    long long start, end;
> +    Range *cur;
> +    char *endptr;
> +
> +    if (siv->ranges) {
> +        return;
> +    }
> +
> +    errno = 0;
> +    do {
> +        start = strtoll(str, &endptr, 0);
> +        if (errno == 0 && endptr > str && INT64_MIN <= start &&
> +            start <= INT64_MAX) {

INT64_MIN <= start && start <= INT64_MAX is dead code (always true,
unless you are on some weird platform where long long is larger than
int64_t).

> +            if (*endptr == '\0') {
> +                cur = g_malloc0(sizeof(*cur));
> +                cur->begin = start;
> +                cur->end = start + 1;
> +                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
> +                                                          range_compare);
> +                cur = NULL;
> +                str = NULL;
> +            } else if (*endptr == '-') {
> +                str = endptr + 1;
> +                end = strtoll(str, &endptr, 0);
> +                if (errno == 0 && endptr > str &&
> +                    INT64_MIN <= end && end <= INT64_MAX && start <= end &&

Another dead line.

...
> +        } else {
> +            goto error;
> +        }
> +    } while (str);

Your code has a bug.  You fail to reset errno = 0 prior to resuming the
next iteration of the while loop, even though you have called
intermediate functions that may have clobbered errno.  You'll need to
submit a followup patch to fix it.


> +static void test_visitor_in_intList(TestInputVisitorData *data,
> +                                    const void *unused)
> +{
> +    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
> +    int16List *res = NULL, *tmp;
> +    Error *errp = NULL;
> +    Visitor *v;
> +    int i = 0;
> +
> +    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");

Should you also test the ability to create ranges of negative numbers,
such as "-3--1", since integers are signed?

> +
> +    visit_type_int16List(v, &res, NULL, &errp);
> +    g_assert(errp == NULL);

It's shorter to write this as one line:

visit_type_int16List(v, &res, NULL, &error_abort);
Michael S. Tsirkin June 18, 2014, 3:13 a.m. UTC | #2 
On Tue, Jun 17, 2014 at 03:36:35PM -0600, Eric Blake wrote:
> On 06/17/2014 11:41 AM, Michael S. Tsirkin wrote:
> > From: Hu Tao <hutao@cn.fujitsu.com>
> > 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > Tested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > MST: split up patch
> > ---
> >  qapi/string-input-visitor.c       | 195 ++++++++++++++++++++++++++++++++++++--
> >  tests/test-string-input-visitor.c |  36 +++++++
> >  2 files changed, 223 insertions(+), 8 deletions(-)
> > 
> 
> > +static void parse_str(StringInputVisitor *siv, Error **errp)
> > +{
> > +    char *str = (char *) siv->string;
> > +    long long start, end;
> > +    Range *cur;
> > +    char *endptr;
> > +
> > +    if (siv->ranges) {
> > +        return;
> > +    }
> > +
> > +    errno = 0;
> > +    do {
> > +        start = strtoll(str, &endptr, 0);
> > +        if (errno == 0 && endptr > str && INT64_MIN <= start &&
> > +            start <= INT64_MAX) {
> 
> INT64_MIN <= start && start <= INT64_MAX is dead code (always true,
> unless you are on some weird platform where long long is larger than
> int64_t).
> 
> > +            if (*endptr == '\0') {
> > +                cur = g_malloc0(sizeof(*cur));
> > +                cur->begin = start;
> > +                cur->end = start + 1;
> > +                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
> > +                                                          range_compare);
> > +                cur = NULL;
> > +                str = NULL;
> > +            } else if (*endptr == '-') {
> > +                str = endptr + 1;
> > +                end = strtoll(str, &endptr, 0);
> > +                if (errno == 0 && endptr > str &&
> > +                    INT64_MIN <= end && end <= INT64_MAX && start <= end &&
> 
> Another dead line.
> 
> ...
> > +        } else {
> > +            goto error;
> > +        }
> > +    } while (str);
> 
> Your code has a bug.  You fail to reset errno = 0 prior to resuming the
> next iteration of the while loop, even though you have called
> intermediate functions that may have clobbered errno.  You'll need to
> submit a followup patch to fix it.
> 
> 
> > +static void test_visitor_in_intList(TestInputVisitorData *data,
> > +                                    const void *unused)
> > +{
> > +    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
> > +    int16List *res = NULL, *tmp;
> > +    Error *errp = NULL;
> > +    Visitor *v;
> > +    int i = 0;
> > +
> > +    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
> 
> Should you also test the ability to create ranges of negative numbers,
> such as "-3--1", since integers are signed?

As the code uses unsigned Range internally, it does not currently work.
I agree we should add a patch to handle negative values and
fail gracefully, but this isn't the only place where we don't:
same applies to most input visitors.

Maybe we should change the default integer to do this check
automatically everywhere.

Are there any integer properties that need negative values?

> > +
> > +    visit_type_int16List(v, &res, NULL, &errp);
> > +    g_assert(errp == NULL);
> 
> It's shorter to write this as one line:
> 
> visit_type_int16List(v, &res, NULL, &error_abort);
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
diff mbox

Patch

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 5780944..0b2490b 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -15,31 +15,205 @@ 
 #include "qapi/visitor-impl.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/option.h"
+#include "qemu/queue.h"
+#include "qemu/range.h"
+
 
 struct StringInputVisitor
 {
     Visitor visitor;
+
+    bool head;
+
+    GList *ranges;
+    GList *cur_range;
+    int64_t cur;
+
     const char *string;
 };
 
+static void parse_str(StringInputVisitor *siv, Error **errp)
+{
+    char *str = (char *) siv->string;
+    long long start, end;
+    Range *cur;
+    char *endptr;
+
+    if (siv->ranges) {
+        return;
+    }
+
+    errno = 0;
+    do {
+        start = strtoll(str, &endptr, 0);
+        if (errno == 0 && endptr > str && INT64_MIN <= start &&
+            start <= INT64_MAX) {
+            if (*endptr == '\0') {
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges, cur,
+                                                          range_compare);
+                cur = NULL;
+                str = NULL;
+            } else if (*endptr == '-') {
+                str = endptr + 1;
+                end = strtoll(str, &endptr, 0);
+                if (errno == 0 && endptr > str &&
+                    INT64_MIN <= end && end <= INT64_MAX && start <= end &&
+                    (start > INT64_MAX - 65536 ||
+                     end < start + 65536)) {
+                    if (*endptr == '\0') {
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
+                        str = NULL;
+                    } else if (*endptr == ',') {
+                        str = endptr + 1;
+                        cur = g_malloc0(sizeof(*cur));
+                        cur->begin = start;
+                        cur->end = end + 1;
+                        siv->ranges =
+                            g_list_insert_sorted_merged(siv->ranges,
+                                                        cur,
+                                                        range_compare);
+                        cur = NULL;
+                    } else {
+                        goto error;
+                    }
+                } else {
+                    goto error;
+                }
+            } else if (*endptr == ',') {
+                str = endptr + 1;
+                cur = g_malloc0(sizeof(*cur));
+                cur->begin = start;
+                cur->end = start + 1;
+                siv->ranges = g_list_insert_sorted_merged(siv->ranges,
+                                                          cur,
+                                                          range_compare);
+                cur = NULL;
+            } else {
+                goto error;
+            }
+        } else {
+            goto error;
+        }
+    } while (str);
+
+    return;
+error:
+    g_list_free_full(siv->ranges, g_free);
+    assert(siv->ranges == NULL);
+}
+
+static void
+start_list(Visitor *v, const char *name, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+
+    parse_str(siv, errp);
+
+    siv->cur_range = g_list_first(siv->ranges);
+    if (siv->cur_range) {
+        Range *r = siv->cur_range->data;
+        if (r) {
+            siv->cur = r->begin;
+        }
+    }
+}
+
+static GenericList *
+next_list(Visitor *v, GenericList **list, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    GenericList **link;
+    Range *r;
+
+    if (!siv->ranges || !siv->cur_range) {
+        return NULL;
+    }
+
+    r = siv->cur_range->data;
+    if (!r) {
+        return NULL;
+    }
+
+    if (siv->cur < r->begin || siv->cur >= r->end) {
+        siv->cur_range = g_list_next(siv->cur_range);
+        if (!siv->cur_range) {
+            return NULL;
+        }
+        r = siv->cur_range->data;
+        if (!r) {
+            return NULL;
+        }
+        siv->cur = r->begin;
+    }
+
+    if (siv->head) {
+        link = list;
+        siv->head = false;
+    } else {
+        link = &(*list)->next;
+    }
+
+    *link = g_malloc0(sizeof **link);
+    return *link;
+}
+
+static void
+end_list(Visitor *v, Error **errp)
+{
+    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    siv->head = true;
+}
+
 static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
     StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
-    char *endp = (char *) siv->string;
-    long long val;
 
-    errno = 0;
-    if (siv->string) {
-        val = strtoll(siv->string, &endp, 0);
-    }
-    if (!siv->string || errno || endp == siv->string || *endp) {
+    if (!siv->string) {
         error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                   "integer");
         return;
     }
 
-    *obj = val;
+    parse_str(siv, errp);
+
+    if (!siv->ranges) {
+        goto error;
+    }
+
+    if (!siv->cur_range) {
+        Range *r;
+
+        siv->cur_range = g_list_first(siv->ranges);
+        if (!siv->cur_range) {
+            goto error;
+        }
+
+        r = siv->cur_range->data;
+        if (!r) {
+            goto error;
+        }
+
+        siv->cur = r->begin;
+    }
+
+    *obj = siv->cur;
+    siv->cur++;
+    return;
+
+error:
+    error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
+              "an int64 value or range");
 }
 
 static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
@@ -140,6 +314,7 @@  Visitor *string_input_get_visitor(StringInputVisitor *v)
 
 void string_input_visitor_cleanup(StringInputVisitor *v)
 {
+    g_list_free_full(v->ranges, g_free);
     g_free(v);
 }
 
@@ -155,8 +330,12 @@  StringInputVisitor *string_input_visitor_new(const char *str)
     v->visitor.type_bool = parse_type_bool;
     v->visitor.type_str = parse_type_str;
     v->visitor.type_number = parse_type_number;
+    v->visitor.start_list = start_list;
+    v->visitor.next_list = next_list;
+    v->visitor.end_list = end_list;
     v->visitor.optional = parse_optional;
 
     v->string = str;
+    v->head = true;
     return v;
 }
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 01a78f0..b01e2f2 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -64,6 +64,35 @@  static void test_visitor_in_int(TestInputVisitorData *data,
     g_assert_cmpint(res, ==, value);
 }
 
+static void test_visitor_in_intList(TestInputVisitorData *data,
+                                    const void *unused)
+{
+    int64_t value[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20};
+    int16List *res = NULL, *tmp;
+    Error *errp = NULL;
+    Visitor *v;
+    int i = 0;
+
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
+
+    visit_type_int16List(v, &res, NULL, &errp);
+    g_assert(errp == NULL);
+    tmp = res;
+    while (i < sizeof(value) / sizeof(value[0])) {
+        g_assert(tmp);
+        g_assert_cmpint(tmp->value, ==, value[i++]);
+        tmp = tmp->next;
+    }
+    g_assert(!tmp);
+
+    tmp = res;
+    while (tmp) {
+        res = res->next;
+        g_free(tmp);
+        tmp = res;
+    }
+}
+
 static void test_visitor_in_bool(TestInputVisitorData *data,
                                  const void *unused)
 {
@@ -170,6 +199,7 @@  static void test_visitor_in_fuzz(TestInputVisitorData *data,
                                  const void *unused)
 {
     int64_t ires;
+    intList *ilres;
     bool bres;
     double nres;
     char *sres;
@@ -196,6 +226,10 @@  static void test_visitor_in_fuzz(TestInputVisitorData *data,
         visitor_input_teardown(data, NULL);
 
         v = visitor_input_test_init(data, buf);
+        visit_type_intList(v, &ilres, NULL, NULL);
+        visitor_input_teardown(data, NULL);
+
+        v = visitor_input_test_init(data, buf);
         visit_type_bool(v, &bres, NULL, NULL);
         visitor_input_teardown(data, NULL);
 
@@ -231,6 +265,8 @@  int main(int argc, char **argv)
 
     input_visitor_test_add("/string-visitor/input/int",
                            &in_visitor_data, test_visitor_in_int);
+    input_visitor_test_add("/string-visitor/input/intList",
+                           &in_visitor_data, test_visitor_in_intList);
     input_visitor_test_add("/string-visitor/input/bool",
                            &in_visitor_data, test_visitor_in_bool);
     input_visitor_test_add("/string-visitor/input/number",