* [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor
@ 2018-11-09 11:02 David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 1/6] cutils: add qemu_strtod() David Hildenbrand
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
Rewrite string-input-visitor to be (hopefully) less ugly. Support
int and uint lists (including ranges, but not implemented via type
"Range").
Virtual walks are now supported and more errors are cought (and some bugs
fixed). Fix and extend the tests. Parsing of uint64_t is now properly
supported.
Importantly, when parsing a list we now return the list and not an
ordered set (we are not an ordered set parser after all). Whoever needs
that can add it on top. As far as I can see, current code can deal with
it but I'll have to look at the details.
Guess once this part here is done, the output visitor is the next thing
to rework.
David Hildenbrand (6):
cutils: add qemu_strtod()
qapi: use qemu_strtod() in string-input-visitor
qapi: rewrite string-input-visitor
test-string-input-visitor: use virtual walk
test-string-input-visitor: split off uint64 list tests
test-string-input-visitor: add range overflow tests
include/qapi/string-input-visitor.h | 3 +-
include/qemu/cutils.h | 1 +
qapi/string-input-visitor.c | 444 ++++++++++++++++------------
tests/test-string-input-visitor.c | 131 +++++---
util/cutils.c | 22 ++
5 files changed, 377 insertions(+), 224 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 1/6] cutils: add qemu_strtod()
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor David Hildenbrand
` (4 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
Let's provide a wrapper for strtod().
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/qemu/cutils.h | 1 +
util/cutils.c | 22 ++++++++++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 7071bfe2d4..84fb9e53c6 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -146,6 +146,7 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
int64_t *result);
int qemu_strtou64(const char *nptr, const char **endptr, int base,
uint64_t *result);
+int qemu_strtod(const char *nptr, const char **endptr, double *result);
int parse_uint(const char *s, unsigned long long *value, char **endptr,
int base);
diff --git a/util/cutils.c b/util/cutils.c
index 698bd315bd..850e3af9ea 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -544,6 +544,28 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
return check_strtox_error(nptr, ep, endptr, errno);
}
+/**
+ * Convert string @nptr to a double.
+ *
+ * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
+ * overflow/underflow.
+ */
+int qemu_strtod(const char *nptr, const char **endptr, double *result)
+{
+ char *ep;
+
+ if (!nptr) {
+ if (endptr) {
+ *endptr = nptr;
+ }
+ return -EINVAL;
+ }
+
+ errno = 0;
+ *result = strtod(nptr, &ep);
+ return check_strtox_error(nptr, ep, endptr, errno);
+}
+
/**
* Searches for the first occurrence of 'c' in 's', and returns a pointer
* to the trailing null byte if none was found.
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 1/6] cutils: add qemu_strtod() David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
2018-11-14 16:09 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor David Hildenbrand
` (3 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
Let's use the new function.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
qapi/string-input-visitor.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b3fdd0827d..dee708d384 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -20,6 +20,7 @@
#include "qemu/option.h"
#include "qemu/queue.h"
#include "qemu/range.h"
+#include "qemu/cutils.h"
struct StringInputVisitor
@@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
Error **errp)
{
StringInputVisitor *siv = to_siv(v);
- char *endp = (char *) siv->string;
double val;
- errno = 0;
- val = strtod(siv->string, &endp);
- if (errno || endp == siv->string || *endp) {
+ if (qemu_strtod(siv->string, NULL, &val)) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"number");
return;
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 1/6] cutils: add qemu_strtod() David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
2018-11-14 17:38 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 4/6] test-string-input-visitor: use virtual walk David Hildenbrand
` (2 subsequent siblings)
5 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
The input visitor has some problems right now, especially
- unsigned type "Range" is used to process signed ranges, resulting in
inconsistent behavior and ugly/magical code
- uint64_t are parsed like int64_t, so big uint64_t values are not
supported and error messages are misleading
- lists/ranges of int64_t are accepted although no list is parsed and
we should rather report an error
- lists/ranges are preparsed using int64_t, making it hard to
implement uint64_t values or uint64_t lists
- types that don't support lists don't bail out
So let's rewrite it by getting rid of usage of the type "Range" and
properly supporting lists of int64_t and uint64_t (including ranges of
both types), fixing the above mentioned issues.
Lists of other types are not supported and will properly report an
error. Virtual walks are now supported.
Tests have to be fixed up:
- Two BUGs were hardcoded that are fixed now
- The string-input-visitor now actually returns a parsed list and not
an ordered set.
While at it, do some smaller style changes (e.g. use g_assert).
Signed-off-by: David Hildenbrand <david@redhat.com>
---
include/qapi/string-input-visitor.h | 3 +-
qapi/string-input-visitor.c | 436 +++++++++++++++++-----------
tests/test-string-input-visitor.c | 18 +-
3 files changed, 264 insertions(+), 193 deletions(-)
diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
index 33551340e3..2c40f7f5a6 100644
--- a/include/qapi/string-input-visitor.h
+++ b/include/qapi/string-input-visitor.h
@@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
/*
* The string input visitor does not implement support for visiting
- * QAPI structs, alternates, null, or arbitrary QTypes. It also
- * requires a non-null list argument to visit_start_list().
+ * QAPI structs, alternates, null, or arbitrary QTypes.
*/
Visitor *string_input_visitor_new(const char *str);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index dee708d384..16da47409e 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -4,10 +4,10 @@
* Copyright Red Hat, Inc. 2012-2016
*
* Author: Paolo Bonzini <pbonzini@redhat.com>
+ * David Hildenbrand <david@redhat.com>
*
* This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
* See the COPYING.LIB file in the top-level directory.
- *
*/
#include "qemu/osdep.h"
@@ -18,21 +18,39 @@
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qnull.h"
#include "qemu/option.h"
-#include "qemu/queue.h"
-#include "qemu/range.h"
#include "qemu/cutils.h"
+typedef enum ListMode {
+ /* no list parsing active / no list expected */
+ LM_NONE,
+ /* we have an unparsed string remaining */
+ LM_UNPARSED,
+ /* we have an unfinished int64 range */
+ LM_INT64_RANGE,
+ /* we have an unfinished uint64 range */
+ LM_UINT64_RANGE,
+ /* we have parsed the string completely and no range is remaining */
+ LM_END,
+} ListMode;
+
+typedef union RangeLimit {
+ int64_t i64;
+ uint64_t u64;
+} RangeLimit;
struct StringInputVisitor
{
Visitor visitor;
- GList *ranges;
- GList *cur_range;
- int64_t cur;
+ /* Porperties related to list processing */
+ ListMode lm;
+ RangeLimit rangeNext;
+ RangeLimit rangeEnd;
+ const char *unparsed_string;
+ void *list;
+ /* the original string to parse */
const char *string;
- void *list; /* Only needed for sanity checking the caller */
};
static StringInputVisitor *to_siv(Visitor *v)
@@ -40,136 +58,48 @@ static StringInputVisitor *to_siv(Visitor *v)
return container_of(v, StringInputVisitor, visitor);
}
-static void free_range(void *range, void *dummy)
-{
- g_free(range);
-}
-
-static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
-{
- char *str = (char *) siv->string;
- long long start, end;
- Range *cur;
- char *endptr;
-
- if (siv->ranges) {
- return 0;
- }
-
- if (!*str) {
- return 0;
- }
-
- do {
- errno = 0;
- start = strtoll(str, &endptr, 0);
- if (errno == 0 && endptr > str) {
- if (*endptr == '\0') {
- cur = g_malloc0(sizeof(*cur));
- range_set_bounds(cur, start, start);
- siv->ranges = range_list_insert(siv->ranges, cur);
- cur = NULL;
- str = NULL;
- } else if (*endptr == '-') {
- str = endptr + 1;
- errno = 0;
- end = strtoll(str, &endptr, 0);
- if (errno == 0 && endptr > str && start <= end &&
- (start > INT64_MAX - 65536 ||
- end < start + 65536)) {
- if (*endptr == '\0') {
- cur = g_malloc0(sizeof(*cur));
- range_set_bounds(cur, start, end);
- siv->ranges = range_list_insert(siv->ranges, cur);
- cur = NULL;
- str = NULL;
- } else if (*endptr == ',') {
- str = endptr + 1;
- cur = g_malloc0(sizeof(*cur));
- range_set_bounds(cur, start, end);
- siv->ranges = range_list_insert(siv->ranges, cur);
- cur = NULL;
- } else {
- goto error;
- }
- } else {
- goto error;
- }
- } else if (*endptr == ',') {
- str = endptr + 1;
- cur = g_malloc0(sizeof(*cur));
- range_set_bounds(cur, start, start);
- siv->ranges = range_list_insert(siv->ranges, cur);
- cur = NULL;
- } else {
- goto error;
- }
- } else {
- goto error;
- }
- } while (str);
-
- return 0;
-error:
- g_list_foreach(siv->ranges, free_range, NULL);
- g_list_free(siv->ranges);
- siv->ranges = NULL;
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
- "an int64 value or range");
- return -1;
-}
-
-static void
-start_list(Visitor *v, const char *name, GenericList **list, size_t size,
- Error **errp)
+static void start_list(Visitor *v, const char *name, GenericList **list,
+ size_t size, Error **errp)
{
StringInputVisitor *siv = to_siv(v);
- /* We don't support visits without a list */
- assert(list);
- siv->list = list;
-
- if (parse_str(siv, name, errp) < 0) {
- *list = NULL;
+ /* Properly set the state for list processing. */
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Already processing a list.");
return;
}
+ siv->list = list;
+ siv->unparsed_string = siv->string;
- siv->cur_range = g_list_first(siv->ranges);
- if (siv->cur_range) {
- Range *r = siv->cur_range->data;
- if (r) {
- siv->cur = range_lob(r);
+ if (!siv->string[0]) {
+ if (list) {
+ *list = NULL;
}
- *list = g_malloc0(size);
+ siv->lm = LM_END;
} else {
- *list = NULL;
+ if (list) {
+ *list = g_malloc0(size);
+ }
+ siv->lm = LM_UNPARSED;
}
}
static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
{
StringInputVisitor *siv = to_siv(v);
- Range *r;
-
- if (!siv->ranges || !siv->cur_range) {
- return NULL;
- }
- r = siv->cur_range->data;
- if (!r) {
+ switch (siv->lm) {
+ case LM_NONE:
+ case LM_END:
+ /* we have reached the end of the list already or have no list */
return NULL;
- }
-
- if (!range_contains(r, siv->cur)) {
- 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 = range_lob(r);
+ case LM_INT64_RANGE:
+ case LM_UINT64_RANGE:
+ case LM_UNPARSED:
+ /* we have an unparsed string or something left in a range */
+ break;
+ default:
+ g_assert_not_reached();
}
tail->next = g_malloc0(size);
@@ -179,88 +109,216 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
static void check_list(Visitor *v, Error **errp)
{
const StringInputVisitor *siv = to_siv(v);
- Range *r;
- GList *cur_range;
- if (!siv->ranges || !siv->cur_range) {
+ switch (siv->lm) {
+ case LM_NONE:
+ error_setg(errp, "Not processing a list.");
+ case LM_INT64_RANGE:
+ case LM_UINT64_RANGE:
+ case LM_UNPARSED:
+ error_setg(errp, "There are elements remaining in the list.");
return;
- }
-
- r = siv->cur_range->data;
- if (!r) {
+ case LM_END:
return;
+ default:
+ g_assert_not_reached();
}
-
- if (!range_contains(r, siv->cur)) {
- cur_range = g_list_next(siv->cur_range);
- if (!cur_range) {
- return;
- }
- r = cur_range->data;
- if (!r) {
- return;
- }
- }
-
- error_setg(errp, "Range contains too many values");
}
static void end_list(Visitor *v, void **obj)
{
StringInputVisitor *siv = to_siv(v);
- assert(siv->list == obj);
+ g_assert(siv->list == obj);
+ siv->list = NULL;
+ siv->unparsed_string = NULL;
+ siv->lm = LM_NONE;
}
-static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
- Error **errp)
+static int try_parse_int64_list_entry(StringInputVisitor *siv, int64_t *obj)
{
- StringInputVisitor *siv = to_siv(v);
+ const char *endptr;
+ int64_t start, end;
- if (parse_str(siv, name, errp) < 0) {
- return;
+ if (qemu_strtoi64(siv->unparsed_string, &endptr, 0, &start)) {
+ return -EINVAL;
}
- if (!siv->ranges) {
- goto error;
+ switch (endptr[0]) {
+ case '\0':
+ siv->lm = LM_END;
+ break;
+ case ',':
+ siv->unparsed_string = endptr + 1;
+ break;
+ case '-':
+ /* parse the end of the range */
+ if (qemu_strtoi64(endptr + 1, &endptr, 0, &end)) {
+ return -EINVAL;
+ }
+ /* we require at least two elements in a range */
+ if (start >= end) {
+ return -EINVAL;
+ }
+ switch (endptr[0]) {
+ case '\0':
+ siv->unparsed_string = endptr;
+ break;
+ case ',':
+ siv->unparsed_string = endptr + 1;
+ break;
+ default:
+ return -EINVAL;
+ }
+ /* we have a proper range */
+ siv->lm = LM_INT64_RANGE;
+ siv->rangeNext.i64 = start + 1;
+ siv->rangeEnd.i64 = end;
+ break;
+ default:
+ return -EINVAL;
}
- if (!siv->cur_range) {
- Range *r;
+ *obj = start;
+ return 0;
+}
- siv->cur_range = g_list_first(siv->ranges);
- if (!siv->cur_range) {
- goto error;
+static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
+ Error **errp)
+{
+ StringInputVisitor *siv = to_siv(v);
+ int64_t val;
+
+ switch (siv->lm) {
+ case LM_NONE:
+ /* just parse a simple int64, bail out if not completely consumed */
+ if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+ name ? name : "null", "int64");
+ return;
}
-
- r = siv->cur_range->data;
- if (!r) {
- goto error;
+ *obj = val;
+ return;
+ case LM_INT64_RANGE:
+ /* return the next element in the range */
+ g_assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
+ *obj = siv->rangeNext.i64++;
+
+ if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
+ /* end of range, check if there is more to parse */
+ if (siv->unparsed_string[0]) {
+ siv->lm = LM_UNPARSED;
+ } else {
+ siv->lm = LM_END;
+ }
}
+ return;
+ case LM_UNPARSED:
+ if (try_parse_int64_list_entry(siv, obj)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+ "list of int64 values or ranges");
+ }
+ return;
+ case LM_END:
+ error_setg(errp, "No more elements in the list.");
+ return;
+ default:
+ error_setg(errp, "Lists don't support mixed types.");
+ return;
+ }
+}
+
+static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
+{
+ const char *endptr;
+ uint64_t start, end;
- siv->cur = range_lob(r);
+ /* parse a simple uint64 or range */
+ if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
+ return -EINVAL;
}
- *obj = siv->cur;
- siv->cur++;
- return;
+ switch (endptr[0]) {
+ case '\0':
+ siv->lm = LM_END;
+ break;
+ case ',':
+ siv->unparsed_string = endptr + 1;
+ break;
+ case '-':
+ /* parse the end of the range */
+ if (qemu_strtou64(endptr + 1, &endptr, 0, &end)) {
+ return -EINVAL;
+ }
+ /* we require at least two elements in a range */
+ if (start >= end) {
+ return -EINVAL;
+ }
+ switch (endptr[0]) {
+ case '\0':
+ siv->unparsed_string = endptr;
+ break;
+ case ',':
+ siv->unparsed_string = endptr + 1;
+ break;
+ default:
+ return -EINVAL;
+ }
+ /* we have a proper range */
+ siv->lm = LM_UINT64_RANGE;
+ siv->rangeNext.u64 = start + 1;
+ siv->rangeEnd.u64 = end;
+ break;
+ default:
+ return -EINVAL;
+ }
-error:
- error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
- "an int64 value or range");
+ *obj = start;
+ return 0;
}
static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
Error **errp)
{
- /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
- int64_t i;
- Error *err = NULL;
- parse_type_int64(v, name, &i, &err);
- if (err) {
- error_propagate(errp, err);
- } else {
- *obj = i;
+ StringInputVisitor *siv = to_siv(v);
+ uint64_t val;
+
+ switch (siv->lm) {
+ case LM_NONE:
+ /* just parse a simple uint64, bail out if not completely consumed */
+ if (qemu_strtou64(siv->string, NULL, 0, &val)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+ "uint64");
+ return;
+ }
+ *obj = val;
+ return;
+ case LM_UINT64_RANGE:
+ /* return the next element in the range */
+ g_assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);
+ *obj = siv->rangeNext.u64++;
+
+ if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
+ /* end of range, check if there is more to parse */
+ if (siv->unparsed_string[0]) {
+ siv->lm = LM_UNPARSED;
+ } else {
+ siv->lm = LM_END;
+ }
+ }
+ return;
+ case LM_UNPARSED:
+ if (try_parse_uint64_list_entry(siv, obj)) {
+ error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+ "list of uint64 values or ranges");
+ }
+ return;
+ case LM_END:
+ error_setg(errp, "No more elements in the list.");
+ return;
+ default:
+ error_setg(errp, "Lists don't support mixed types.");
+ return;
}
}
@@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
Error *err = NULL;
uint64_t val;
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Lists not supported for type \"size\"");
+ return;
+ }
+
parse_option_size(name, siv->string, &val, &err);
if (err) {
error_propagate(errp, err);
@@ -285,6 +348,11 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
{
StringInputVisitor *siv = to_siv(v);
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Lists not supported for type \"boolean\"");
+ return;
+ }
+
if (!strcasecmp(siv->string, "on") ||
!strcasecmp(siv->string, "yes") ||
!strcasecmp(siv->string, "true")) {
@@ -307,6 +375,11 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
{
StringInputVisitor *siv = to_siv(v);
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Lists not supported for type \"string\"");
+ return;
+ }
+
*obj = g_strdup(siv->string);
}
@@ -316,6 +389,11 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
StringInputVisitor *siv = to_siv(v);
double val;
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Lists not supported for type \"number\"");
+ return;
+ }
+
if (qemu_strtod(siv->string, NULL, &val)) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"number");
@@ -332,7 +410,12 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
*obj = NULL;
- if (!siv->string || siv->string[0]) {
+ if (siv->lm != LM_NONE) {
+ error_setg(errp, "Lists not supported for type \"null\"");
+ return;
+ }
+
+ if (siv->string[0]) {
error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"null");
return;
@@ -345,8 +428,6 @@ static void string_input_free(Visitor *v)
{
StringInputVisitor *siv = to_siv(v);
- g_list_foreach(siv->ranges, free_range, NULL);
- g_list_free(siv->ranges);
g_free(siv);
}
@@ -354,7 +435,7 @@ Visitor *string_input_visitor_new(const char *str)
{
StringInputVisitor *v;
- assert(str);
+ g_assert(str);
v = g_malloc0(sizeof(*v));
v->visitor.type = VISITOR_INPUT;
@@ -372,5 +453,6 @@ Visitor *string_input_visitor_new(const char *str)
v->visitor.free = string_input_free;
v->string = str;
+ v->lm = LM_NONE;
return &v->visitor;
}
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 88e0e1aa9a..0a4152f79d 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
uint64List *tail;
int i;
- /* BUG: unsigned numbers above INT64_MAX don't work */
- for (i = 0; i < n; i++) {
- if (expected[i] > INT64_MAX) {
- Error *err = NULL;
- visit_type_uint64List(v, NULL, &res, &err);
- error_free_or_abort(&err);
- return;
- }
- }
-
visit_type_uint64List(v, NULL, &res, &error_abort);
tail = res;
for (i = 0; i < n; i++) {
@@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
static void test_visitor_in_intList(TestInputVisitorData *data,
const void *unused)
{
- /* Note: the visitor *sorts* ranges *unsigned* */
- int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
+ int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
+ 6, 7, 8 };
int64_t expect2[] = { 32767, -32768, -32767 };
- int64_t expect3[] = { INT64_MAX, INT64_MIN };
+ int64_t expect3[] = { INT64_MIN, INT64_MAX };
uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
int64List *res = NULL;
@@ -189,7 +179,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
visit_type_int64(v, NULL, &tail->value, &err);
g_assert_cmpint(tail->value, ==, 0);
visit_type_int64(v, NULL, &val, &err);
- g_assert_cmpint(val, ==, 1); /* BUG */
+ error_free_or_abort(&err);
visit_check_list(v, &error_abort);
visit_end_list(v, (void **)&res);
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 4/6] test-string-input-visitor: use virtual walk
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
` (2 preceding siblings ...)
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 6/6] test-string-input-visitor: add range overflow tests David Hildenbrand
5 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
We now support virtual walks, so use that instead.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
tests/test-string-input-visitor.c | 36 +++++++++++--------------------
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 0a4152f79d..2f6360e9ca 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -114,7 +114,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
int64List *res = NULL;
- int64List *tail;
Visitor *v;
int64_t val;
@@ -151,39 +150,28 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
v = visitor_input_test_init(data, "0,2-3");
- /* Would be simpler if the visitor genuinely supported virtual walks */
- visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
- &error_abort);
- tail = res;
- visit_type_int64(v, NULL, &tail->value, &error_abort);
- g_assert_cmpint(tail->value, ==, 0);
- tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
- g_assert(tail);
- visit_type_int64(v, NULL, &tail->value, &error_abort);
- g_assert_cmpint(tail->value, ==, 2);
- tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
- g_assert(tail);
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_int64(v, NULL, &val, &error_abort);
+ g_assert_cmpint(val, ==, 0);
+ visit_type_int64(v, NULL, &val, &error_abort);
+ g_assert_cmpint(val, ==, 2);
visit_check_list(v, &err);
error_free_or_abort(&err);
- visit_end_list(v, (void **)&res);
-
- qapi_free_int64List(res);
+ visit_end_list(v, NULL);
/* Visit beyond end of list */
+
v = visitor_input_test_init(data, "0");
- visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
- &error_abort);
- tail = res;
- visit_type_int64(v, NULL, &tail->value, &err);
- g_assert_cmpint(tail->value, ==, 0);
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_int64(v, NULL, &val, &err);
+ g_assert_cmpint(val, ==, 0);
visit_type_int64(v, NULL, &val, &err);
error_free_or_abort(&err);
- visit_check_list(v, &error_abort);
- visit_end_list(v, (void **)&res);
- qapi_free_int64List(res);
+ visit_check_list(v, &error_abort);
+ visit_end_list(v, NULL);
}
static void test_visitor_in_bool(TestInputVisitorData *data,
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
` (3 preceding siblings ...)
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 4/6] test-string-input-visitor: use virtual walk David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
2018-11-14 16:21 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 6/6] test-string-input-visitor: add range overflow tests David Hildenbrand
5 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
Basically copy all int64 list tests but adapt them to work on uint64
instead.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 4 deletions(-)
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 2f6360e9ca..731094f789 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
6, 7, 8 };
int64_t expect2[] = { 32767, -32768, -32767 };
int64_t expect3[] = { INT64_MIN, INT64_MAX };
- uint64_t expect4[] = { UINT64_MAX };
Error *err = NULL;
int64List *res = NULL;
Visitor *v;
@@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
"-9223372036854775808,9223372036854775807");
check_ilist(v, expect3, ARRAY_SIZE(expect3));
- v = visitor_input_test_init(data, "18446744073709551615");
- check_ulist(v, expect4, ARRAY_SIZE(expect4));
-
/* Empty list */
v = visitor_input_test_init(data, "");
@@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
visit_end_list(v, NULL);
}
+static void test_visitor_in_uintList(TestInputVisitorData *data,
+ const void *unused)
+{
+ uint64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
+ 6, 7, 8 };
+ uint64_t expect2[] = { 32767, -32768, -32767 };
+ uint64_t expect3[] = { UINT64_MAX };
+ Error *err = NULL;
+ uint64List *res = NULL;
+ Visitor *v;
+ uint64_t val;
+
+ /* Valid lists */
+
+ v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
+ check_ulist(v, expect1, ARRAY_SIZE(expect1));
+
+ v = visitor_input_test_init(data, "32767,-32768--32767");
+ check_ulist(v, expect2, ARRAY_SIZE(expect2));
+
+ v = visitor_input_test_init(data, "18446744073709551615");
+ check_ulist(v, expect3, ARRAY_SIZE(expect3));
+
+ /* Empty list */
+
+ v = visitor_input_test_init(data, "");
+ visit_type_uint64List(v, NULL, &res, &error_abort);
+ g_assert(!res);
+
+ /* Not a list */
+
+ v = visitor_input_test_init(data, "not an uint list");
+
+ visit_type_uint64List(v, NULL, &res, &err);
+ error_free_or_abort(&err);
+ g_assert(!res);
+
+ /* Unvisited list tail */
+
+ v = visitor_input_test_init(data, "0,2-3");
+
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_uint64(v, NULL, &val, &error_abort);
+ g_assert_cmpuint(val, ==, 0);
+ visit_type_uint64(v, NULL, &val, &error_abort);
+ g_assert_cmpuint(val, ==, 2);
+
+ visit_check_list(v, &err);
+ error_free_or_abort(&err);
+ visit_end_list(v, NULL);
+
+ /* Visit beyond end of list */
+
+ v = visitor_input_test_init(data, "0");
+
+ visit_start_list(v, NULL, NULL, 0, &error_abort);
+ visit_type_uint64(v, NULL, &val, &err);
+ g_assert_cmpuint(val, ==, 0);
+ visit_type_uint64(v, NULL, &val, &err);
+ error_free_or_abort(&err);
+
+ visit_check_list(v, &error_abort);
+ visit_end_list(v, NULL);
+}
+
static void test_visitor_in_bool(TestInputVisitorData *data,
const void *unused)
{
@@ -334,6 +395,8 @@ int main(int argc, char **argv)
&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/uintList",
+ &in_visitor_data, test_visitor_in_uintList);
input_visitor_test_add("/string-visitor/input/bool",
&in_visitor_data, test_visitor_in_bool);
input_visitor_test_add("/string-visitor/input/number",
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH RFC 6/6] test-string-input-visitor: add range overflow tests
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
` (4 preceding siblings ...)
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests David Hildenbrand
@ 2018-11-09 11:02 ` David Hildenbrand
5 siblings, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2018-11-09 11:02 UTC (permalink / raw)
To: qemu-devel
Cc: Markus Armbruster, Michael Roth, Eric Blake, Paolo Bonzini,
David Hildenbrand
Let's make sure that the range handling code can properly deal with
ranges that end at the biggest possible number.
Signed-off-by: David Hildenbrand <david@redhat.com>
---
tests/test-string-input-visitor.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 731094f789..809bd59fca 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -111,6 +111,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
6, 7, 8 };
int64_t expect2[] = { 32767, -32768, -32767 };
int64_t expect3[] = { INT64_MIN, INT64_MAX };
+ int64_t expect4[] = { INT64_MAX - 2, INT64_MAX - 1, INT64_MAX };
Error *err = NULL;
int64List *res = NULL;
Visitor *v;
@@ -128,6 +129,10 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
"-9223372036854775808,9223372036854775807");
check_ilist(v, expect3, ARRAY_SIZE(expect3));
+ v = visitor_input_test_init(data,
+ "9223372036854775805-9223372036854775807");
+ check_ilist(v, expect4, ARRAY_SIZE(expect4));
+
/* Empty list */
v = visitor_input_test_init(data, "");
@@ -177,6 +182,7 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,
6, 7, 8 };
uint64_t expect2[] = { 32767, -32768, -32767 };
uint64_t expect3[] = { UINT64_MAX };
+ uint64_t expect4[] = { UINT64_MAX - 2, UINT64_MAX - 1, UINT64_MAX };
Error *err = NULL;
uint64List *res = NULL;
Visitor *v;
@@ -193,6 +199,10 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,
v = visitor_input_test_init(data, "18446744073709551615");
check_ulist(v, expect3, ARRAY_SIZE(expect3));
+ v = visitor_input_test_init(data,
+ "18446744073709551613-18446744073709551615");
+ check_ulist(v, expect4, ARRAY_SIZE(expect4));
+
/* Empty list */
v = visitor_input_test_init(data, "");
--
2.17.2
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor David Hildenbrand
@ 2018-11-14 16:09 ` Markus Armbruster
2018-11-15 11:09 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2018-11-14 16:09 UTC (permalink / raw)
To: David Hildenbrand; +Cc: qemu-devel, Paolo Bonzini, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> Let's use the new function.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> qapi/string-input-visitor.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..dee708d384 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
> #include "qemu/option.h"
> #include "qemu/queue.h"
> #include "qemu/range.h"
> +#include "qemu/cutils.h"
>
>
> struct StringInputVisitor
> @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
> Error **errp)
> {
> StringInputVisitor *siv = to_siv(v);
> - char *endp = (char *) siv->string;
> double val;
>
> - errno = 0;
> - val = strtod(siv->string, &endp);
> - if (errno || endp == siv->string || *endp) {
> + if (qemu_strtod(siv->string, NULL, &val)) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "number");
> return;
Three more: in qobject-input-visitor.c's
qobject_input_type_number_keyval(), cutil.c's do_strtosz(), and
json-parser.c's parse_literal().
The latter doesn't check for errors since the lexer ensures the input is
sane. Overflow can still happen, and is silently ignored. Feel free
not to convert this one.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests David Hildenbrand
@ 2018-11-14 16:21 ` Markus Armbruster
2018-11-14 20:03 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2018-11-14 16:21 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Paolo Bonzini, Markus Armbruster, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> Basically copy all int64 list tests but adapt them to work on uint64
> instead.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
> index 2f6360e9ca..731094f789 100644
> --- a/tests/test-string-input-visitor.c
> +++ b/tests/test-string-input-visitor.c
> @@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
> 6, 7, 8 };
> int64_t expect2[] = { 32767, -32768, -32767 };
> int64_t expect3[] = { INT64_MIN, INT64_MAX };
> - uint64_t expect4[] = { UINT64_MAX };
> Error *err = NULL;
> int64List *res = NULL;
> Visitor *v;
> @@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
> "-9223372036854775808,9223372036854775807");
> check_ilist(v, expect3, ARRAY_SIZE(expect3));
>
> - v = visitor_input_test_init(data, "18446744073709551615");
> - check_ulist(v, expect4, ARRAY_SIZE(expect4));
> -
Hmm. Testing behavior for this input is still worthwhile, isn't it?
The use of check_ulist() here is admittedly unclean.
> /* Empty list */
>
> v = visitor_input_test_init(data, "");
> @@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
> visit_end_list(v, NULL);
> }
>
> +static void test_visitor_in_uintList(TestInputVisitorData *data,
> + const void *unused)
> +{
> + uint64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
Please wrap this line a bit earlier.
> + 6, 7, 8 };
> + uint64_t expect2[] = { 32767, -32768, -32767 };
> + uint64_t expect3[] = { UINT64_MAX };
> + Error *err = NULL;
> + uint64List *res = NULL;
> + Visitor *v;
> + uint64_t val;
> +
> + /* Valid lists */
> +
> + v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
> + check_ulist(v, expect1, ARRAY_SIZE(expect1));
> +
> + v = visitor_input_test_init(data, "32767,-32768--32767");
> + check_ulist(v, expect2, ARRAY_SIZE(expect2));
> +
> + v = visitor_input_test_init(data, "18446744073709551615");
> + check_ulist(v, expect3, ARRAY_SIZE(expect3));
Test behavior for large negative numbers?
> +
> + /* Empty list */
> +
> + v = visitor_input_test_init(data, "");
> + visit_type_uint64List(v, NULL, &res, &error_abort);
> + g_assert(!res);
> +
> + /* Not a list */
> +
> + v = visitor_input_test_init(data, "not an uint list");
> +
> + visit_type_uint64List(v, NULL, &res, &err);
> + error_free_or_abort(&err);
> + g_assert(!res);
> +
> + /* Unvisited list tail */
> +
> + v = visitor_input_test_init(data, "0,2-3");
> +
> + visit_start_list(v, NULL, NULL, 0, &error_abort);
> + visit_type_uint64(v, NULL, &val, &error_abort);
> + g_assert_cmpuint(val, ==, 0);
> + visit_type_uint64(v, NULL, &val, &error_abort);
> + g_assert_cmpuint(val, ==, 2);
> +
> + visit_check_list(v, &err);
> + error_free_or_abort(&err);
> + visit_end_list(v, NULL);
> +
> + /* Visit beyond end of list */
> +
> + v = visitor_input_test_init(data, "0");
> +
> + visit_start_list(v, NULL, NULL, 0, &error_abort);
> + visit_type_uint64(v, NULL, &val, &err);
> + g_assert_cmpuint(val, ==, 0);
> + visit_type_uint64(v, NULL, &val, &err);
> + error_free_or_abort(&err);
> +
> + visit_check_list(v, &error_abort);
> + visit_end_list(v, NULL);
> +}
> +
> static void test_visitor_in_bool(TestInputVisitorData *data,
> const void *unused)
> {
> @@ -334,6 +395,8 @@ int main(int argc, char **argv)
> &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/uintList",
> + &in_visitor_data, test_visitor_in_uintList);
> input_visitor_test_add("/string-visitor/input/bool",
> &in_visitor_data, test_visitor_in_bool);
> input_visitor_test_add("/string-visitor/input/number",
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor David Hildenbrand
@ 2018-11-14 17:38 ` Markus Armbruster
2018-11-14 19:56 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2018-11-14 17:38 UTC (permalink / raw)
To: David Hildenbrand
Cc: qemu-devel, Paolo Bonzini, Markus Armbruster, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> The input visitor has some problems right now, especially
> - unsigned type "Range" is used to process signed ranges, resulting in
> inconsistent behavior and ugly/magical code
> - uint64_t are parsed like int64_t, so big uint64_t values are not
> supported and error messages are misleading
> - lists/ranges of int64_t are accepted although no list is parsed and
> we should rather report an error
> - lists/ranges are preparsed using int64_t, making it hard to
> implement uint64_t values or uint64_t lists
> - types that don't support lists don't bail out
Known weirdness: empty list is invalid (test-string-input-visitor.c
demonstates). Your patch doesn't change that (or else it would update
the test). Should it be changed?
>
> So let's rewrite it by getting rid of usage of the type "Range" and
> properly supporting lists of int64_t and uint64_t (including ranges of
> both types), fixing the above mentioned issues.
>
> Lists of other types are not supported and will properly report an
> error. Virtual walks are now supported.
>
> Tests have to be fixed up:
> - Two BUGs were hardcoded that are fixed now
> - The string-input-visitor now actually returns a parsed list and not
> an ordered set.
>
> While at it, do some smaller style changes (e.g. use g_assert).
Please don't replace assert() by g_assert() in files that consistently
use assert() now.
In my opinion, g_assert() is one of the cases where GLib pointlessly
reinvents wheels that have been carrying load since forever.
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> include/qapi/string-input-visitor.h | 3 +-
> qapi/string-input-visitor.c | 436 +++++++++++++++++-----------
> tests/test-string-input-visitor.c | 18 +-
> 3 files changed, 264 insertions(+), 193 deletions(-)
>
> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
> index 33551340e3..2c40f7f5a6 100644
> --- a/include/qapi/string-input-visitor.h
> +++ b/include/qapi/string-input-visitor.h
> @@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
>
> /*
> * The string input visitor does not implement support for visiting
> - * QAPI structs, alternates, null, or arbitrary QTypes. It also
> - * requires a non-null list argument to visit_start_list().
> + * QAPI structs, alternates, null, or arbitrary QTypes.
Preexisting: neglects to spell out the list limitations, i.e. can do
only flat lists of integers. Care do fix that, too?
> */
> Visitor *string_input_visitor_new(const char *str);
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index dee708d384..16da47409e 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -4,10 +4,10 @@
> * Copyright Red Hat, Inc. 2012-2016
> *
> * Author: Paolo Bonzini <pbonzini@redhat.com>
> + * David Hildenbrand <david@redhat.com>
> *
> * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> * See the COPYING.LIB file in the top-level directory.
> - *
> */
>
> #include "qemu/osdep.h"
> @@ -18,21 +18,39 @@
> #include "qapi/qmp/qerror.h"
> #include "qapi/qmp/qnull.h"
> #include "qemu/option.h"
> -#include "qemu/queue.h"
> -#include "qemu/range.h"
> #include "qemu/cutils.h"
>
> +typedef enum ListMode {
> + /* no list parsing active / no list expected */
> + LM_NONE,
> + /* we have an unparsed string remaining */
> + LM_UNPARSED,
> + /* we have an unfinished int64 range */
> + LM_INT64_RANGE,
> + /* we have an unfinished uint64 range */
> + LM_UINT64_RANGE,
> + /* we have parsed the string completely and no range is remaining */
> + LM_END,
> +} ListMode;
> +
> +typedef union RangeLimit {
> + int64_t i64;
> + uint64_t u64;
> +} RangeLimit;
>
> struct StringInputVisitor
> {
> Visitor visitor;
>
> - GList *ranges;
> - GList *cur_range;
> - int64_t cur;
> + /* Porperties related to list processing */
"Properties"
Suggest
/* List parsing state */
> + ListMode lm;
> + RangeLimit rangeNext;
> + RangeLimit rangeEnd;
RangeLimit is a good name for rangeEnd, but not so hot for rangeNext.
Naming is hard.
> + const char *unparsed_string;
> + void *list;
You drop /* Only needed for sanity checking the caller */.
Double-checking: is that not true anymore?
>
> + /* the original string to parse */
> const char *string;
> - void *list; /* Only needed for sanity checking the caller */
> };
>
> static StringInputVisitor *to_siv(Visitor *v)
> @@ -40,136 +58,48 @@ static StringInputVisitor *to_siv(Visitor *v)
> return container_of(v, StringInputVisitor, visitor);
> }
>
> -static void free_range(void *range, void *dummy)
> -{
> - g_free(range);
> -}
> -
> -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> -{
> - char *str = (char *) siv->string;
> - long long start, end;
> - Range *cur;
> - char *endptr;
> -
> - if (siv->ranges) {
> - return 0;
> - }
> -
> - if (!*str) {
> - return 0;
> - }
> -
> - do {
> - errno = 0;
> - start = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str) {
> - if (*endptr == '\0') {
> - cur = g_malloc0(sizeof(*cur));
> - range_set_bounds(cur, start, start);
> - siv->ranges = range_list_insert(siv->ranges, cur);
> - cur = NULL;
> - str = NULL;
> - } else if (*endptr == '-') {
> - str = endptr + 1;
> - errno = 0;
> - end = strtoll(str, &endptr, 0);
> - if (errno == 0 && endptr > str && start <= end &&
> - (start > INT64_MAX - 65536 ||
> - end < start + 65536)) {
> - if (*endptr == '\0') {
> - cur = g_malloc0(sizeof(*cur));
> - range_set_bounds(cur, start, end);
> - siv->ranges = range_list_insert(siv->ranges, cur);
> - cur = NULL;
> - str = NULL;
> - } else if (*endptr == ',') {
> - str = endptr + 1;
> - cur = g_malloc0(sizeof(*cur));
> - range_set_bounds(cur, start, end);
> - siv->ranges = range_list_insert(siv->ranges, cur);
> - cur = NULL;
> - } else {
> - goto error;
> - }
> - } else {
> - goto error;
> - }
> - } else if (*endptr == ',') {
> - str = endptr + 1;
> - cur = g_malloc0(sizeof(*cur));
> - range_set_bounds(cur, start, start);
> - siv->ranges = range_list_insert(siv->ranges, cur);
> - cur = NULL;
> - } else {
> - goto error;
> - }
> - } else {
> - goto error;
> - }
> - } while (str);
> -
> - return 0;
> -error:
> - g_list_foreach(siv->ranges, free_range, NULL);
> - g_list_free(siv->ranges);
> - siv->ranges = NULL;
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> - "an int64 value or range");
> - return -1;
> -}
Good riddance!
> -
> -static void
> -start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> - Error **errp)
> +static void start_list(Visitor *v, const char *name, GenericList **list,
> + size_t size, Error **errp)
> {
> StringInputVisitor *siv = to_siv(v);
>
> - /* We don't support visits without a list */
> - assert(list);
> - siv->list = list;
> -
> - if (parse_str(siv, name, errp) < 0) {
> - *list = NULL;
> + /* Properly set the state for list processing. */
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Already processing a list.");
Drop the period, please. error_setg()'s contract stipulates:
* The resulting message should be a single phrase, with no newline or
* trailing punctuation.
More of the same below.
> return;
> }
> + siv->list = list;
> + siv->unparsed_string = siv->string;
>
> - siv->cur_range = g_list_first(siv->ranges);
> - if (siv->cur_range) {
> - Range *r = siv->cur_range->data;
> - if (r) {
> - siv->cur = range_lob(r);
> + if (!siv->string[0]) {
> + if (list) {
> + *list = NULL;
> }
> - *list = g_malloc0(size);
> + siv->lm = LM_END;
> } else {
> - *list = NULL;
> + if (list) {
> + *list = g_malloc0(size);
> + }
> + siv->lm = LM_UNPARSED;
> }
> }
Works a bit like qobject_input_start_list() now. Good.
>
> static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
> {
> StringInputVisitor *siv = to_siv(v);
> - Range *r;
> -
> - if (!siv->ranges || !siv->cur_range) {
> - return NULL;
> - }
>
> - r = siv->cur_range->data;
> - if (!r) {
> + switch (siv->lm) {
> + case LM_NONE:
> + case LM_END:
> + /* we have reached the end of the list already or have no list */
> return NULL;
Hmm. Is there a way to reach case LM_NONE other than a programming
error? If no, we should abort then. To do so, fold LM_NONE into the
default case below.
> - }
> -
> - if (!range_contains(r, siv->cur)) {
> - 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 = range_lob(r);
> + case LM_INT64_RANGE:
> + case LM_UINT64_RANGE:
> + case LM_UNPARSED:
> + /* we have an unparsed string or something left in a range */
> + break;
> + default:
> + g_assert_not_reached();
abort() suffices, and is more consistent with the rest of qapi/.
> }
>
> tail->next = g_malloc0(size);
> @@ -179,88 +109,216 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
> static void check_list(Visitor *v, Error **errp)
> {
> const StringInputVisitor *siv = to_siv(v);
> - Range *r;
> - GList *cur_range;
>
> - if (!siv->ranges || !siv->cur_range) {
> + switch (siv->lm) {
> + case LM_NONE:
> + error_setg(errp, "Not processing a list.");
Same question as for next_list().
> + case LM_INT64_RANGE:
> + case LM_UINT64_RANGE:
> + case LM_UNPARSED:
> + error_setg(errp, "There are elements remaining in the list.");
Hmm. qobject_input_check_list() reports "Only %u list elements expected
in %s" here. Looks unintuitive, until you remember how it's normally
used: to parse user input. The error gets reported when the parsing
didn't consume the whole list. Can only happen with a virtual walk.
And when it happens, the issue to report is "you provided a longer list
than I can accept". qobject_input_check_list() does exactly that. Your
message is less clear.
> return;
> - }
> -
> - r = siv->cur_range->data;
> - if (!r) {
> + case LM_END:
> return;
> + default:
> + g_assert_not_reached();
> }
> -
> - if (!range_contains(r, siv->cur)) {
> - cur_range = g_list_next(siv->cur_range);
> - if (!cur_range) {
> - return;
> - }
> - r = cur_range->data;
> - if (!r) {
> - return;
> - }
> - }
> -
> - error_setg(errp, "Range contains too many values");
> }
>
> static void end_list(Visitor *v, void **obj)
> {
> StringInputVisitor *siv = to_siv(v);
>
> - assert(siv->list == obj);
I suspect state LM_NONE is also a programming error here. If that's
correct, let's assert(siv->lm != LM_NONE).
> + g_assert(siv->list == obj);
> + siv->list = NULL;
> + siv->unparsed_string = NULL;
> + siv->lm = LM_NONE;
> }
>
> -static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
> - Error **errp)
> +static int try_parse_int64_list_entry(StringInputVisitor *siv, int64_t *obj)
> {
> - StringInputVisitor *siv = to_siv(v);
> + const char *endptr;
> + int64_t start, end;
>
> - if (parse_str(siv, name, errp) < 0) {
> - return;
> + if (qemu_strtoi64(siv->unparsed_string, &endptr, 0, &start)) {
> + return -EINVAL;
> }
>
> - if (!siv->ranges) {
> - goto error;
> + switch (endptr[0]) {
> + case '\0':
> + siv->lm = LM_END;
> + break;
> + case ',':
> + siv->unparsed_string = endptr + 1;
> + break;
> + case '-':
> + /* parse the end of the range */
> + if (qemu_strtoi64(endptr + 1, &endptr, 0, &end)) {
> + return -EINVAL;
> + }
> + /* we require at least two elements in a range */
> + if (start >= end) {
> + return -EINVAL;
> + }
> + switch (endptr[0]) {
> + case '\0':
> + siv->unparsed_string = endptr;
> + break;
> + case ',':
> + siv->unparsed_string = endptr + 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> + /* we have a proper range */
> + siv->lm = LM_INT64_RANGE;
> + siv->rangeNext.i64 = start + 1;
> + siv->rangeEnd.i64 = end;
> + break;
> + default:
> + return -EINVAL;
> }
>
> - if (!siv->cur_range) {
> - Range *r;
> + *obj = start;
> + return 0;
> +}
>
> - siv->cur_range = g_list_first(siv->ranges);
> - if (!siv->cur_range) {
> - goto error;
> +static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
> + Error **errp)
> +{
> + StringInputVisitor *siv = to_siv(v);
> + int64_t val;
> +
> + switch (siv->lm) {
> + case LM_NONE:
> + /* just parse a simple int64, bail out if not completely consumed */
> + if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> + name ? name : "null", "int64");
> + return;
> }
> -
> - r = siv->cur_range->data;
> - if (!r) {
> - goto error;
> + *obj = val;
> + return;
> + case LM_INT64_RANGE:
> + /* return the next element in the range */
> + g_assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
> + *obj = siv->rangeNext.i64++;
> +
> + if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
> + /* end of range, check if there is more to parse */
> + if (siv->unparsed_string[0]) {
> + siv->lm = LM_UNPARSED;
> + } else {
> + siv->lm = LM_END;
> + }
> }
Are you sure this is kosher?
if siv->rangeNext.i64 and siv->rangeEnd are both initially INT64_MAX,
siv->rangeNext++ wraps around to INT64_MIN. Except when the compiler
exploits undefined behavior.
You could try something like
g_assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
*obj = siv->rangeNext.i64;
if (siv->rangeNext.i64 == siv->rangeEnd.i64) {
/* end of range, check if there is more to parse */
if (siv->unparsed_string[0]) {
siv->lm = LM_UNPARSED;
} else {
siv->lm = LM_END;
}
} else {
siv->rangeNext.i64++;
}
Another alternative might be to iterate in uint64_t (and dispense with
union RangeLimit), then convert to int64_t.
> + return;
> + case LM_UNPARSED:
> + if (try_parse_int64_list_entry(siv, obj)) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> + "list of int64 values or ranges");
> + }
I figure I'd make try_parse_int64_list_entry() just parse, and on
success fall through to case LM_INT64_RANGE. But your solution works,
too.
> + return;
> + case LM_END:
> + error_setg(errp, "No more elements in the list.");
This is the buddy of check_list() case LM_UNPARSED etc. There, input
provides more list members than we expect. Here, input provides less.
Again, the error is less clear than the one the QObject input visitor
reports: "Parameter '%s' is missing".
Same for the unsigned copy below.
> + return;
> + default:
> + error_setg(errp, "Lists don't support mixed types.");
Is this a programming error?
Same for the unsigned copy below.
> + return;
> + }
> +}
> +
> +static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
> +{
> + const char *endptr;
> + uint64_t start, end;
>
> - siv->cur = range_lob(r);
> + /* parse a simple uint64 or range */
> + if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
> + return -EINVAL;
> }
>
> - *obj = siv->cur;
> - siv->cur++;
> - return;
> + switch (endptr[0]) {
> + case '\0':
> + siv->lm = LM_END;
> + break;
> + case ',':
> + siv->unparsed_string = endptr + 1;
> + break;
> + case '-':
> + /* parse the end of the range */
> + if (qemu_strtou64(endptr + 1, &endptr, 0, &end)) {
> + return -EINVAL;
> + }
> + /* we require at least two elements in a range */
> + if (start >= end) {
> + return -EINVAL;
> + }
> + switch (endptr[0]) {
> + case '\0':
> + siv->unparsed_string = endptr;
> + break;
> + case ',':
> + siv->unparsed_string = endptr + 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> + /* we have a proper range */
> + siv->lm = LM_UINT64_RANGE;
> + siv->rangeNext.u64 = start + 1;
> + siv->rangeEnd.u64 = end;
> + break;
> + default:
> + return -EINVAL;
> + }
>
> -error:
> - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> - "an int64 value or range");
> + *obj = start;
> + return 0;
> }
>
> static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
> Error **errp)
> {
> - /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
> - int64_t i;
> - Error *err = NULL;
> - parse_type_int64(v, name, &i, &err);
> - if (err) {
> - error_propagate(errp, err);
> - } else {
> - *obj = i;
> + StringInputVisitor *siv = to_siv(v);
> + uint64_t val;
> +
> + switch (siv->lm) {
> + case LM_NONE:
> + /* just parse a simple uint64, bail out if not completely consumed */
> + if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> + "uint64");
> + return;
> + }
> + *obj = val;
> + return;
> + case LM_UINT64_RANGE:
> + /* return the next element in the range */
> + g_assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);
> + *obj = siv->rangeNext.u64++;
> +
> + if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
> + /* end of range, check if there is more to parse */
> + if (siv->unparsed_string[0]) {
> + siv->lm = LM_UNPARSED;
> + } else {
> + siv->lm = LM_END;
> + }
> + }
Unsigned wraps around fine. But when you fix the signed code, you
should probably keep this unsigned code as similar as practical.
> + return;
> + case LM_UNPARSED:
> + if (try_parse_uint64_list_entry(siv, obj)) {
> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> + "list of uint64 values or ranges");
> + }
> + return;
> + case LM_END:
> + error_setg(errp, "No more elements in the list.");
> + return;
> + default:
> + error_setg(errp, "Lists don't support mixed types.");
> + return;
> }
> }
>
> @@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
> Error *err = NULL;
> uint64_t val;
>
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Lists not supported for type \"size\"");
> + return;
> + }
> +
Programming error? More of the same below.
> parse_option_size(name, siv->string, &val, &err);
> if (err) {
> error_propagate(errp, err);
> @@ -285,6 +348,11 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
> {
> StringInputVisitor *siv = to_siv(v);
>
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Lists not supported for type \"boolean\"");
> + return;
> + }
> +
> if (!strcasecmp(siv->string, "on") ||
> !strcasecmp(siv->string, "yes") ||
> !strcasecmp(siv->string, "true")) {
> @@ -307,6 +375,11 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
> {
> StringInputVisitor *siv = to_siv(v);
>
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Lists not supported for type \"string\"");
> + return;
> + }
> +
> *obj = g_strdup(siv->string);
> }
>
> @@ -316,6 +389,11 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
> StringInputVisitor *siv = to_siv(v);
> double val;
>
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Lists not supported for type \"number\"");
> + return;
> + }
> +
> if (qemu_strtod(siv->string, NULL, &val)) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "number");
> @@ -332,7 +410,12 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
>
> *obj = NULL;
>
> - if (!siv->string || siv->string[0]) {
> + if (siv->lm != LM_NONE) {
> + error_setg(errp, "Lists not supported for type \"null\"");
> + return;
> + }
> +
> + if (siv->string[0]) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
> "null");
> return;
> @@ -345,8 +428,6 @@ static void string_input_free(Visitor *v)
> {
> StringInputVisitor *siv = to_siv(v);
>
> - g_list_foreach(siv->ranges, free_range, NULL);
> - g_list_free(siv->ranges);
> g_free(siv);
> }
>
> @@ -354,7 +435,7 @@ Visitor *string_input_visitor_new(const char *str)
> {
> StringInputVisitor *v;
>
> - assert(str);
> + g_assert(str);
> v = g_malloc0(sizeof(*v));
>
> v->visitor.type = VISITOR_INPUT;
> @@ -372,5 +453,6 @@ Visitor *string_input_visitor_new(const char *str)
> v->visitor.free = string_input_free;
>
> v->string = str;
> + v->lm = LM_NONE;
> return &v->visitor;
> }
> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
> index 88e0e1aa9a..0a4152f79d 100644
> --- a/tests/test-string-input-visitor.c
> +++ b/tests/test-string-input-visitor.c
> @@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
> uint64List *tail;
> int i;
>
> - /* BUG: unsigned numbers above INT64_MAX don't work */
> - for (i = 0; i < n; i++) {
> - if (expected[i] > INT64_MAX) {
> - Error *err = NULL;
> - visit_type_uint64List(v, NULL, &res, &err);
> - error_free_or_abort(&err);
> - return;
> - }
> - }
> -
> visit_type_uint64List(v, NULL, &res, &error_abort);
> tail = res;
> for (i = 0; i < n; i++) {
> @@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
> static void test_visitor_in_intList(TestInputVisitorData *data,
> const void *unused)
> {
> - /* Note: the visitor *sorts* ranges *unsigned* */
> - int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
> + int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
Please wrap this line a bit earlier.
> + 6, 7, 8 };
> int64_t expect2[] = { 32767, -32768, -32767 };
> - int64_t expect3[] = { INT64_MAX, INT64_MIN };
> + int64_t expect3[] = { INT64_MIN, INT64_MAX };
Did you mean to change this line?
> uint64_t expect4[] = { UINT64_MAX };
> Error *err = NULL;
> int64List *res = NULL;
> @@ -189,7 +179,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
> visit_type_int64(v, NULL, &tail->value, &err);
> g_assert_cmpint(tail->value, ==, 0);
> visit_type_int64(v, NULL, &val, &err);
> - g_assert_cmpint(val, ==, 1); /* BUG */
> + error_free_or_abort(&err);
Which of the problems listed in commit message is this?
> visit_check_list(v, &error_abort);
> visit_end_list(v, (void **)&res);
Please don't let my review comments discourage you! This is so much
better than the code we have now, and feels pretty close to ready.
Thanks a lot!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-14 17:38 ` Markus Armbruster
@ 2018-11-14 19:56 ` David Hildenbrand
2018-11-15 9:48 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-14 19:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Michael Roth
On 14.11.18 18:38, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> The input visitor has some problems right now, especially
>> - unsigned type "Range" is used to process signed ranges, resulting in
>> inconsistent behavior and ugly/magical code
>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>> supported and error messages are misleading
>> - lists/ranges of int64_t are accepted although no list is parsed and
>> we should rather report an error
>> - lists/ranges are preparsed using int64_t, making it hard to
>> implement uint64_t values or uint64_t lists
>> - types that don't support lists don't bail out
>
> Known weirdness: empty list is invalid (test-string-input-visitor.c
> demonstates). Your patch doesn't change that (or else it would update
> the test). Should it be changed?
>
I don't change the test, so the old behavior still works.
(empty string -> error)
>>
>> So let's rewrite it by getting rid of usage of the type "Range" and
>> properly supporting lists of int64_t and uint64_t (including ranges of
>> both types), fixing the above mentioned issues.
>>
>> Lists of other types are not supported and will properly report an
>> error. Virtual walks are now supported.
>>
>> Tests have to be fixed up:
>> - Two BUGs were hardcoded that are fixed now
>> - The string-input-visitor now actually returns a parsed list and not
>> an ordered set.
>>
>> While at it, do some smaller style changes (e.g. use g_assert).
>
> Please don't replace assert() by g_assert() in files that consistently
> use assert() now.
Alright, will use assert() and don't replace. (I thought g_assert() was
the new fancy thing to do it in QEMU).
>
> In my opinion, g_assert() is one of the cases where GLib pointlessly
> reinvents wheels that have been carrying load since forever.
>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> include/qapi/string-input-visitor.h | 3 +-
>> qapi/string-input-visitor.c | 436 +++++++++++++++++-----------
>> tests/test-string-input-visitor.c | 18 +-
>> 3 files changed, 264 insertions(+), 193 deletions(-)
>>
>> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
>> index 33551340e3..2c40f7f5a6 100644
>> --- a/include/qapi/string-input-visitor.h
>> +++ b/include/qapi/string-input-visitor.h
>> @@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
>>
>> /*
>> * The string input visitor does not implement support for visiting
>> - * QAPI structs, alternates, null, or arbitrary QTypes. It also
>> - * requires a non-null list argument to visit_start_list().
>> + * QAPI structs, alternates, null, or arbitrary QTypes.
>
> Preexisting: neglects to spell out the list limitations, i.e. can do
> only flat lists of integers. Care do fix that, too?
Added "Only flat lists of integers (int64/uint64) are supported."
>
>> */
>> Visitor *string_input_visitor_new(const char *str);
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index dee708d384..16da47409e 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -4,10 +4,10 @@
>> * Copyright Red Hat, Inc. 2012-2016
>> *
>> * Author: Paolo Bonzini <pbonzini@redhat.com>
>> + * David Hildenbrand <david@redhat.com>
>> *
>> * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>> * See the COPYING.LIB file in the top-level directory.
>> - *
>> */
>>
>> #include "qemu/osdep.h"
>> @@ -18,21 +18,39 @@
>> #include "qapi/qmp/qerror.h"
>> #include "qapi/qmp/qnull.h"
>> #include "qemu/option.h"
>> -#include "qemu/queue.h"
>> -#include "qemu/range.h"
>> #include "qemu/cutils.h"
>>
>> +typedef enum ListMode {
>> + /* no list parsing active / no list expected */
>> + LM_NONE,
>> + /* we have an unparsed string remaining */
>> + LM_UNPARSED,
>> + /* we have an unfinished int64 range */
>> + LM_INT64_RANGE,
>> + /* we have an unfinished uint64 range */
>> + LM_UINT64_RANGE,
>> + /* we have parsed the string completely and no range is remaining */
>> + LM_END,
>> +} ListMode;
>> +
>> +typedef union RangeLimit {
>> + int64_t i64;
>> + uint64_t u64;
>> +} RangeLimit;
>>
>> struct StringInputVisitor
>> {
>> Visitor visitor;
>>
>> - GList *ranges;
>> - GList *cur_range;
>> - int64_t cur;
>> + /* Porperties related to list processing */
>
> "Properties"
>
> Suggest
>
> /* List parsing state */
Yes, will use that.
>
>> + ListMode lm;
>> + RangeLimit rangeNext;
>> + RangeLimit rangeEnd;
>
> RangeLimit is a good name for rangeEnd, but not so hot for rangeNext.
> Naming is hard.
Initially I had two unnamed unions to not have to give a name ;)
RangeElement ? Will use that for now.
>
>> + const char *unparsed_string;
>> + void *list;
>
> You drop /* Only needed for sanity checking the caller */.
> Double-checking: is that not true anymore?
I consider such comments bad as they can easily become outdated (and
IMHO don't help anybody). But I can leave it here, don't really care.
[...]
>> + error_setg(errp, "Already processing a list.");
>
> Drop the period, please. error_setg()'s contract stipulates:
>
> * The resulting message should be a single phrase, with no newline or
> * trailing punctuation.
>
> More of the same below.
All dots dropped :)
[...]
>> static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>> {
>> StringInputVisitor *siv = to_siv(v);
>> - Range *r;
>> -
>> - if (!siv->ranges || !siv->cur_range) {
>> - return NULL;
>> - }
>>
>> - r = siv->cur_range->data;
>> - if (!r) {
>> + switch (siv->lm) {
>> + case LM_NONE:
>> + case LM_END:
>> + /* we have reached the end of the list already or have no list */
>> return NULL;
>
> Hmm. Is there a way to reach case LM_NONE other than a programming
> error? If no, we should abort then. To do so, fold LM_NONE into the
> default case below.
Alright, all programming errors will be encoded as asserts.
>
>> - }
>> -
>> - if (!range_contains(r, siv->cur)) {
>> - 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 = range_lob(r);
>> + case LM_INT64_RANGE:
>> + case LM_UINT64_RANGE:
>> + case LM_UNPARSED:
>> + /* we have an unparsed string or something left in a range */
>> + break;
>> + default:
>> + g_assert_not_reached();
>
> abort() suffices, and is more consistent with the rest of qapi/.
Right, qapi is special ;)
Will use s/g_assert/assert/
s/g_assert_not_reached/abort/
>
>> }
>>
>> tail->next = g_malloc0(size);
>> @@ -179,88 +109,216 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>> static void check_list(Visitor *v, Error **errp)
>> {
>> const StringInputVisitor *siv = to_siv(v);
>> - Range *r;
>> - GList *cur_range;
>>
>> - if (!siv->ranges || !siv->cur_range) {
>> + switch (siv->lm) {
>> + case LM_NONE:
>> + error_setg(errp, "Not processing a list.");
>
> Same question as for next_list().
abort() it is :) -> default case
>
>> + case LM_INT64_RANGE:
>> + case LM_UINT64_RANGE:
>> + case LM_UNPARSED:
>> + error_setg(errp, "There are elements remaining in the list.");
>
> Hmm. qobject_input_check_list() reports "Only %u list elements expected
> in %s" here. Looks unintuitive, until you remember how it's normally
> used: to parse user input. The error gets reported when the parsing
> didn't consume the whole list. Can only happen with a virtual walk.
> And when it happens, the issue to report is "you provided a longer list
> than I can accept". qobject_input_check_list() does exactly that. Your
> message is less clear.
We don't keep track of element count (and I don't like adding it just to
print a error message).
What about "Less list elements expected"? That at least matches the
other error.
>
>> return;
>> - }
>> -
>> - r = siv->cur_range->data;
>> - if (!r) {
>> + case LM_END:
>> return;
>> + default:
>> + g_assert_not_reached();
>> }
>> -
>> - if (!range_contains(r, siv->cur)) {
>> - cur_range = g_list_next(siv->cur_range);
>> - if (!cur_range) {
>> - return;
>> - }
>> - r = cur_range->data;
>> - if (!r) {
>> - return;
>> - }
>> - }
>> -
>> - error_setg(errp, "Range contains too many values");
>> }
>>
>> static void end_list(Visitor *v, void **obj)
>> {
>> StringInputVisitor *siv = to_siv(v);
>>
>> - assert(siv->list == obj);
>
> I suspect state LM_NONE is also a programming error here. If that's
> correct, let's assert(siv->lm != LM_NONE).
Indeed, added.
[...]
>> + if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
>> + /* end of range, check if there is more to parse */
>> + if (siv->unparsed_string[0]) {
>> + siv->lm = LM_UNPARSED;
>> + } else {
>> + siv->lm = LM_END;
>> + }
>> }
>
> Are you sure this is kosher?
>
> if siv->rangeNext.i64 and siv->rangeEnd are both initially INT64_MAX,
> siv->rangeNext++ wraps around to INT64_MIN. Except when the compiler
> exploits undefined behavior.
*obj == INT64_MAX checks exactly that
(it is the old value of siv->rangeNext.i64 before a possible wraparound).
BTW I added a test case for exactly that as I had the BUG you described
in the code ;)
[...]
>> + return;
>> + case LM_UNPARSED:
>> + if (try_parse_int64_list_entry(siv, obj)) {
>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> + "list of int64 values or ranges");
>> + }
>
> I figure I'd make try_parse_int64_list_entry() just parse, and on
> success fall through to case LM_INT64_RANGE. But your solution works,
> too.
Then we would have to represent even single values as ranges, which is
something I'd like to avoid.
>
>> + return;
>> + case LM_END:
>> + error_setg(errp, "No more elements in the list.");
>
> This is the buddy of check_list() case LM_UNPARSED etc. There, input
> provides more list members than we expect. Here, input provides less.
> Again, the error is less clear than the one the QObject input visitor
> reports: "Parameter '%s' is missing".
>
> Same for the unsigned copy below.
Will also use "Less list elements expected".
"Parameter '%s' is missing" does not really apply to list elements
without a name. We can discuss when I resend.
>
>> + return;
>> + default:
>> + error_setg(errp, "Lists don't support mixed types.");
>
> Is this a programming error?
>
> Same for the unsigned copy below.
abort() it is.
[...]
>> +
>> + if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
>> + /* end of range, check if there is more to parse */
>> + if (siv->unparsed_string[0]) {
>> + siv->lm = LM_UNPARSED;
>> + } else {
>> + siv->lm = LM_END;
>> + }
>> + }
>
> Unsigned wraps around fine. But when you fix the signed code, you
> should probably keep this unsigned code as similar as practical.
Again, *obj == UINT64_MAX checks this.
>
>> + return;
>> + case LM_UNPARSED:
>> + if (try_parse_uint64_list_entry(siv, obj)) {
>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> + "list of uint64 values or ranges");
>> + }
>> + return;
>> + case LM_END:
>> + error_setg(errp, "No more elements in the list.");
>> + return;
>> + default:
>> + error_setg(errp, "Lists don't support mixed types.");
>> + return;
>> }
>> }
>>
>> @@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
>> Error *err = NULL;
>> uint64_t val;
>>
>> + if (siv->lm != LM_NONE) {
>> + error_setg(errp, "Lists not supported for type \"size\"");
>> + return;
>> + }
>> +
>
> Programming error? More of the same below.
assert() it is.
[...]
>> visit_type_uint64List(v, NULL, &res, &error_abort);
>> tail = res;
>> for (i = 0; i < n; i++) {
>> @@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
>> static void test_visitor_in_intList(TestInputVisitorData *data,
>> const void *unused)
>> {
>> - /* Note: the visitor *sorts* ranges *unsigned* */
>> - int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
>> + int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
>
> Please wrap this line a bit earlier.
Will do.
>
>> + 6, 7, 8 };
>> int64_t expect2[] = { 32767, -32768, -32767 };
>> - int64_t expect3[] = { INT64_MAX, INT64_MIN };
>> + int64_t expect3[] = { INT64_MIN, INT64_MAX };
>
> Did you mean to change this line?
>
Yes, we don't sort the list anymore.
>> uint64_t expect4[] = { UINT64_MAX };
>> Error *err = NULL;
>> int64List *res = NULL;
>> @@ -189,7 +179,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>> visit_type_int64(v, NULL, &tail->value, &err);
>> g_assert_cmpint(tail->value, ==, 0);
>> visit_type_int64(v, NULL, &val, &err);
>> - g_assert_cmpint(val, ==, 1); /* BUG */
>> + error_free_or_abort(&err);
>
> Which of the problems listed in commit message is this?
I guess the not mentioned "visiting beyond the end of a list is not
handled properly" bug (most probably due to old range handling). Will
add it to the description,
>
>> visit_check_list(v, &error_abort);
>> visit_end_list(v, (void **)&res);
>
> Please don't let my review comments discourage you! This is so much
> better than the code we have now, and feels pretty close to ready.
> Thanks a lot!
>
I appreciate people reviewing my code :) So don't worry, I'll do
requested changes and resend, then we'll see what we are still missing.
BTW: I'll add support for single-element ranges (1-1) as the previous
code seemed to accepted that.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests
2018-11-14 16:21 ` Markus Armbruster
@ 2018-11-14 20:03 ` David Hildenbrand
2018-11-15 9:59 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-14 20:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Michael Roth
On 14.11.18 17:21, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> Basically copy all int64 list tests but adapt them to work on uint64
>> instead.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
>> 1 file changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
>> index 2f6360e9ca..731094f789 100644
>> --- a/tests/test-string-input-visitor.c
>> +++ b/tests/test-string-input-visitor.c
>> @@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>> 6, 7, 8 };
>> int64_t expect2[] = { 32767, -32768, -32767 };
>> int64_t expect3[] = { INT64_MIN, INT64_MAX };
>> - uint64_t expect4[] = { UINT64_MAX };
>> Error *err = NULL;
>> int64List *res = NULL;
>> Visitor *v;
>> @@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>> "-9223372036854775808,9223372036854775807");
>> check_ilist(v, expect3, ARRAY_SIZE(expect3));
>>
>> - v = visitor_input_test_init(data, "18446744073709551615");
>> - check_ulist(v, expect4, ARRAY_SIZE(expect4));
>> -
>
> Hmm. Testing behavior for this input is still worthwhile, isn't it?
>
> The use of check_ulist() here is admittedly unclean.
This check has been moved to the other function where we test ulists.
Or do you want this check here to test again ilist and see that an error
gets reported as the value is too big? Will add such range checks.
>
>> /* Empty list */
>>
>> v = visitor_input_test_init(data, "");
>> @@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>> visit_end_list(v, NULL);
>> }
>>
>> +static void test_visitor_in_uintList(TestInputVisitorData *data,
>> + const void *unused)
>> +{
>> + uint64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
>
> Please wrap this line a bit earlier.
Yes.
>
>> + 6, 7, 8 };
>> + uint64_t expect2[] = { 32767, -32768, -32767 };
>> + uint64_t expect3[] = { UINT64_MAX };
>> + Error *err = NULL;
>> + uint64List *res = NULL;
>> + Visitor *v;
>> + uint64_t val;
>> +
>> + /* Valid lists */
>> +
>> + v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>> + check_ulist(v, expect1, ARRAY_SIZE(expect1));
>> +
>> + v = visitor_input_test_init(data, "32767,-32768--32767");
>> + check_ulist(v, expect2, ARRAY_SIZE(expect2));
>> +
>> + v = visitor_input_test_init(data, "18446744073709551615");
>> + check_ulist(v, expect3, ARRAY_SIZE(expect3));
>
> Test behavior for large negative numbers?
Yes, will add.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-14 19:56 ` David Hildenbrand
@ 2018-11-15 9:48 ` Markus Armbruster
2018-11-15 10:16 ` David Hildenbrand
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2018-11-15 9:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Markus Armbruster, Paolo Bonzini, qemu-devel, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> On 14.11.18 18:38, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> The input visitor has some problems right now, especially
>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>> inconsistent behavior and ugly/magical code
>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>> supported and error messages are misleading
>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>> we should rather report an error
>>> - lists/ranges are preparsed using int64_t, making it hard to
>>> implement uint64_t values or uint64_t lists
>>> - types that don't support lists don't bail out
>>
>> Known weirdness: empty list is invalid (test-string-input-visitor.c
>> demonstates). Your patch doesn't change that (or else it would update
>> the test). Should it be changed?
>>
>
> I don't change the test, so the old behavior still works.
> (empty string -> error)
Understand. Design question: should it remain an error? Feel free to
declare the question out of scope for this patch.
>>> So let's rewrite it by getting rid of usage of the type "Range" and
>>> properly supporting lists of int64_t and uint64_t (including ranges of
>>> both types), fixing the above mentioned issues.
>>>
>>> Lists of other types are not supported and will properly report an
>>> error. Virtual walks are now supported.
>>>
>>> Tests have to be fixed up:
>>> - Two BUGs were hardcoded that are fixed now
>>> - The string-input-visitor now actually returns a parsed list and not
>>> an ordered set.
>>>
>>> While at it, do some smaller style changes (e.g. use g_assert).
>>
>> Please don't replace assert() by g_assert() in files that consistently
>> use assert() now.
>
> Alright, will use assert() and don't replace. (I thought g_assert() was
> the new fancy thing to do it in QEMU).
>
>> In my opinion, g_assert() is one of the cases where GLib pointlessly
>> reinvents wheels that have been carrying load since forever.
>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> include/qapi/string-input-visitor.h | 3 +-
>>> qapi/string-input-visitor.c | 436 +++++++++++++++++-----------
>>> tests/test-string-input-visitor.c | 18 +-
>>> 3 files changed, 264 insertions(+), 193 deletions(-)
>>>
>>> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
>>> index 33551340e3..2c40f7f5a6 100644
>>> --- a/include/qapi/string-input-visitor.h
>>> +++ b/include/qapi/string-input-visitor.h
>>> @@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
>>>
>>> /*
>>> * The string input visitor does not implement support for visiting
>>> - * QAPI structs, alternates, null, or arbitrary QTypes. It also
>>> - * requires a non-null list argument to visit_start_list().
>>> + * QAPI structs, alternates, null, or arbitrary QTypes.
>>
>> Preexisting: neglects to spell out the list limitations, i.e. can do
>> only flat lists of integers. Care do fix that, too?
>
> Added "Only flat lists of integers (int64/uint64) are supported."
Hmm, do lists of narrower integer types also work? I guess they do: the
narrower visit_type_*int*() call v->type_*int64() via
visit_type_*intN().
Lists of type size are expressly excluded, in parse_type_size() below.
That's okay, we can lift the restriction when it gets in the way.
>>> */
>>> Visitor *string_input_visitor_new(const char *str);
>>>
>>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>>> index dee708d384..16da47409e 100644
>>> --- a/qapi/string-input-visitor.c
>>> +++ b/qapi/string-input-visitor.c
>>> @@ -4,10 +4,10 @@
>>> * Copyright Red Hat, Inc. 2012-2016
>>> *
>>> * Author: Paolo Bonzini <pbonzini@redhat.com>
>>> + * David Hildenbrand <david@redhat.com>
>>> *
>>> * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>>> * See the COPYING.LIB file in the top-level directory.
>>> - *
>>> */
>>>
>>> #include "qemu/osdep.h"
>>> @@ -18,21 +18,39 @@
>>> #include "qapi/qmp/qerror.h"
>>> #include "qapi/qmp/qnull.h"
>>> #include "qemu/option.h"
>>> -#include "qemu/queue.h"
>>> -#include "qemu/range.h"
>>> #include "qemu/cutils.h"
>>>
>>> +typedef enum ListMode {
>>> + /* no list parsing active / no list expected */
>>> + LM_NONE,
>>> + /* we have an unparsed string remaining */
>>> + LM_UNPARSED,
>>> + /* we have an unfinished int64 range */
>>> + LM_INT64_RANGE,
>>> + /* we have an unfinished uint64 range */
>>> + LM_UINT64_RANGE,
>>> + /* we have parsed the string completely and no range is remaining */
>>> + LM_END,
>>> +} ListMode;
>>> +
>>> +typedef union RangeLimit {
>>> + int64_t i64;
>>> + uint64_t u64;
>>> +} RangeLimit;
>>>
>>> struct StringInputVisitor
>>> {
>>> Visitor visitor;
>>>
>>> - GList *ranges;
>>> - GList *cur_range;
>>> - int64_t cur;
>>> + /* Porperties related to list processing */
>>
>> "Properties"
>>
>> Suggest
>>
>> /* List parsing state */
>
> Yes, will use that.
>
>>
>>> + ListMode lm;
>>> + RangeLimit rangeNext;
>>> + RangeLimit rangeEnd;
>>
>> RangeLimit is a good name for rangeEnd, but not so hot for rangeNext.
>> Naming is hard.
>
> Initially I had two unnamed unions to not have to give a name ;)
>
> RangeElement ? Will use that for now.
Works for me.
>>
>>> + const char *unparsed_string;
>>> + void *list;
>>
>> You drop /* Only needed for sanity checking the caller */.
>> Double-checking: is that not true anymore?
>
> I consider such comments bad as they can easily become outdated (and
> IMHO don't help anybody). But I can leave it here, don't really care.
I'm okay with dropping it.
> [...]
>
>>> + error_setg(errp, "Already processing a list.");
>>
>> Drop the period, please. error_setg()'s contract stipulates:
>>
>> * The resulting message should be a single phrase, with no newline or
>> * trailing punctuation.
>>
>> More of the same below.
>
> All dots dropped :)
>
> [...]
>>> static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>>> {
>>> StringInputVisitor *siv = to_siv(v);
>>> - Range *r;
>>> -
>>> - if (!siv->ranges || !siv->cur_range) {
>>> - return NULL;
>>> - }
>>>
>>> - r = siv->cur_range->data;
>>> - if (!r) {
>>> + switch (siv->lm) {
>>> + case LM_NONE:
>>> + case LM_END:
>>> + /* we have reached the end of the list already or have no list */
>>> return NULL;
>>
>> Hmm. Is there a way to reach case LM_NONE other than a programming
>> error? If no, we should abort then. To do so, fold LM_NONE into the
>> default case below.
>
> Alright, all programming errors will be encoded as asserts.
Yes, please. In my experience, distinguishing between programming
errors and other errors really helps with understanding the code.
>>
>>> - }
>>> -
>>> - if (!range_contains(r, siv->cur)) {
>>> - 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 = range_lob(r);
>>> + case LM_INT64_RANGE:
>>> + case LM_UINT64_RANGE:
>>> + case LM_UNPARSED:
>>> + /* we have an unparsed string or something left in a range */
>>> + break;
>>> + default:
>>> + g_assert_not_reached();
>>
>> abort() suffices, and is more consistent with the rest of qapi/.
>
> Right, qapi is special ;)
Sort of. I like keeping things consistent and simple. Some parts of
QEMU consistently use plain assert(), some parts consistently use
g_assert(), and some mix them up with abandon. I'm fighting a rearguard
action against the latter.
> Will use s/g_assert/assert/
> s/g_assert_not_reached/abort/
Thanks!
>>
>>> }
>>>
>>> tail->next = g_malloc0(size);
>>> @@ -179,88 +109,216 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>>> static void check_list(Visitor *v, Error **errp)
>>> {
>>> const StringInputVisitor *siv = to_siv(v);
>>> - Range *r;
>>> - GList *cur_range;
>>>
>>> - if (!siv->ranges || !siv->cur_range) {
>>> + switch (siv->lm) {
>>> + case LM_NONE:
>>> + error_setg(errp, "Not processing a list.");
>>
>> Same question as for next_list().
>
> abort() it is :) -> default case
>
>>
>>> + case LM_INT64_RANGE:
>>> + case LM_UINT64_RANGE:
>>> + case LM_UNPARSED:
>>> + error_setg(errp, "There are elements remaining in the list.");
>>
>> Hmm. qobject_input_check_list() reports "Only %u list elements expected
>> in %s" here. Looks unintuitive, until you remember how it's normally
>> used: to parse user input. The error gets reported when the parsing
>> didn't consume the whole list. Can only happen with a virtual walk.
>> And when it happens, the issue to report is "you provided a longer list
>> than I can accept". qobject_input_check_list() does exactly that. Your
>> message is less clear.
>
> We don't keep track of element count (and I don't like adding it just to
> print a error message).
The QObject input visitor has more of a need for identifying the
offending part of the input precisely, because it supports nesting.
> What about "Less list elements expected"? That at least matches the
> other error.
Good enough. I'd say "fewer", though.
>>
>>> return;
>>> - }
>>> -
>>> - r = siv->cur_range->data;
>>> - if (!r) {
>>> + case LM_END:
>>> return;
>>> + default:
>>> + g_assert_not_reached();
>>> }
>>> -
>>> - if (!range_contains(r, siv->cur)) {
>>> - cur_range = g_list_next(siv->cur_range);
>>> - if (!cur_range) {
>>> - return;
>>> - }
>>> - r = cur_range->data;
>>> - if (!r) {
>>> - return;
>>> - }
>>> - }
>>> -
>>> - error_setg(errp, "Range contains too many values");
>>> }
>>>
>>> static void end_list(Visitor *v, void **obj)
>>> {
>>> StringInputVisitor *siv = to_siv(v);
>>>
>>> - assert(siv->list == obj);
>>
>> I suspect state LM_NONE is also a programming error here. If that's
>> correct, let's assert(siv->lm != LM_NONE).
>
> Indeed, added.
>
> [...]
>>> + if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
>>> + /* end of range, check if there is more to parse */
>>> + if (siv->unparsed_string[0]) {
>>> + siv->lm = LM_UNPARSED;
>>> + } else {
>>> + siv->lm = LM_END;
>>> + }
>>> }
>>
>> Are you sure this is kosher?
>>
>> if siv->rangeNext.i64 and siv->rangeEnd are both initially INT64_MAX,
>> siv->rangeNext++ wraps around to INT64_MIN. Except when the compiler
>> exploits undefined behavior.
>
> *obj == INT64_MAX checks exactly that
>
> (it is the old value of siv->rangeNext.i64 before a possible wraparound).
By the time you check *obj == INT64_MAX, you already executed
siv->rangeNext++ with undefined behavior.
But wait... your code is fine because we compile with -fwrapv.
> BTW I added a test case for exactly that as I had the BUG you described
> in the code ;)
>
> [...]
>>> + return;
>>> + case LM_UNPARSED:
>>> + if (try_parse_int64_list_entry(siv, obj)) {
>>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>>> + "list of int64 values or ranges");
>>> + }
>>
>> I figure I'd make try_parse_int64_list_entry() just parse, and on
>> success fall through to case LM_INT64_RANGE. But your solution works,
>> too.
>
> Then we would have to represent even single values as ranges, which is
> something I'd like to avoid.
Your artistic license applies.
>>> + return;
>>> + case LM_END:
>>> + error_setg(errp, "No more elements in the list.");
>>
>> This is the buddy of check_list() case LM_UNPARSED etc. There, input
>> provides more list members than we expect. Here, input provides less.
>> Again, the error is less clear than the one the QObject input visitor
>> reports: "Parameter '%s' is missing".
>>
>> Same for the unsigned copy below.
>
> Will also use "Less list elements expected".
Good enough. I'd say "fewer", though.
> "Parameter '%s' is missing" does not really apply to list elements
> without a name. We can discuss when I resend.
The QObject input visitor uses the list index as name, see
full_name_nth(). I'm *not* suggesting you implement that for the string
input visitor.
>>
>>> + return;
>>> + default:
>>> + error_setg(errp, "Lists don't support mixed types.");
>>
>> Is this a programming error?
>>
>> Same for the unsigned copy below.
>
> abort() it is.
>
> [...]
>>> +
>>> + if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
>>> + /* end of range, check if there is more to parse */
>>> + if (siv->unparsed_string[0]) {
>>> + siv->lm = LM_UNPARSED;
>>> + } else {
>>> + siv->lm = LM_END;
>>> + }
>>> + }
>>
>> Unsigned wraps around fine. But when you fix the signed code, you
>> should probably keep this unsigned code as similar as practical.
>
> Again, *obj == UINT64_MAX checks this.
>
>>
>>> + return;
>>> + case LM_UNPARSED:
>>> + if (try_parse_uint64_list_entry(siv, obj)) {
>>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>>> + "list of uint64 values or ranges");
>>> + }
>>> + return;
>>> + case LM_END:
>>> + error_setg(errp, "No more elements in the list.");
>>> + return;
>>> + default:
>>> + error_setg(errp, "Lists don't support mixed types.");
>>> + return;
>>> }
>>> }
>>>
>>> @@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
>>> Error *err = NULL;
>>> uint64_t val;
>>>
>>> + if (siv->lm != LM_NONE) {
>>> + error_setg(errp, "Lists not supported for type \"size\"");
>>> + return;
>>> + }
>>> +
>>
>> Programming error? More of the same below.
>
> assert() it is.
>
> [...]
>>> visit_type_uint64List(v, NULL, &res, &error_abort);
>>> tail = res;
>>> for (i = 0; i < n; i++) {
>>> @@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
>>> static void test_visitor_in_intList(TestInputVisitorData *data,
>>> const void *unused)
>>> {
>>> - /* Note: the visitor *sorts* ranges *unsigned* */
>>> - int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
>>> + int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
>>
>> Please wrap this line a bit earlier.
>
> Will do.
>
>>
>>> + 6, 7, 8 };
>>> int64_t expect2[] = { 32767, -32768, -32767 };
>>> - int64_t expect3[] = { INT64_MAX, INT64_MIN };
>>> + int64_t expect3[] = { INT64_MIN, INT64_MAX };
>>
>> Did you mean to change this line?
>>
>
> Yes, we don't sort the list anymore.
D'oh!
>>> uint64_t expect4[] = { UINT64_MAX };
>>> Error *err = NULL;
>>> int64List *res = NULL;
>>> @@ -189,7 +179,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>>> visit_type_int64(v, NULL, &tail->value, &err);
>>> g_assert_cmpint(tail->value, ==, 0);
>>> visit_type_int64(v, NULL, &val, &err);
>>> - g_assert_cmpint(val, ==, 1); /* BUG */
>>> + error_free_or_abort(&err);
>>
>> Which of the problems listed in commit message is this?
>
> I guess the not mentioned "visiting beyond the end of a list is not
> handled properly" bug (most probably due to old range handling). Will
> add it to the description,
Thanks!
>>
>>> visit_check_list(v, &error_abort);
>>> visit_end_list(v, (void **)&res);
>>
>> Please don't let my review comments discourage you! This is so much
>> better than the code we have now, and feels pretty close to ready.
>> Thanks a lot!
>>
>
> I appreciate people reviewing my code :) So don't worry, I'll do
> requested changes and resend, then we'll see what we are still missing.
>
> BTW: I'll add support for single-element ranges (1-1) as the previous
> code seemed to accepted that.
Good idea.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests
2018-11-14 20:03 ` David Hildenbrand
@ 2018-11-15 9:59 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2018-11-15 9:59 UTC (permalink / raw)
To: David Hildenbrand; +Cc: Paolo Bonzini, qemu-devel, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> On 14.11.18 17:21, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> Basically copy all int64 list tests but adapt them to work on uint64
>>> instead.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>> tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
>>> 1 file changed, 67 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
>>> index 2f6360e9ca..731094f789 100644
>>> --- a/tests/test-string-input-visitor.c
>>> +++ b/tests/test-string-input-visitor.c
>>> @@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>>> 6, 7, 8 };
>>> int64_t expect2[] = { 32767, -32768, -32767 };
>>> int64_t expect3[] = { INT64_MIN, INT64_MAX };
>>> - uint64_t expect4[] = { UINT64_MAX };
>>> Error *err = NULL;
>>> int64List *res = NULL;
>>> Visitor *v;
>>> @@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>>> "-9223372036854775808,9223372036854775807");
>>> check_ilist(v, expect3, ARRAY_SIZE(expect3));
>>>
>>> - v = visitor_input_test_init(data, "18446744073709551615");
>>> - check_ulist(v, expect4, ARRAY_SIZE(expect4));
>>> -
>>
>> Hmm. Testing behavior for this input is still worthwhile, isn't it?
>>
>> The use of check_ulist() here is admittedly unclean.
>
> This check has been moved to the other function where we test ulists.
You're right, you didn't reduce test coverage. I got confused.
But there's an opportunity to extend coverage: test
visit_type_int64List() for this input. Separate patch. If you put it
before PATCH 3, it'll ensure PATCH 3 doesn't change behavior for this
case. Suggestion, not demand. Use your judgement.
> Or do you want this check here to test again ilist and see that an error
> gets reported as the value is too big? Will add such range checks.
Be careful with new range checks, they can easily break backward
compatibility. Deciding whether the break is acceptable then requires
analysis.
>>
>>> /* Empty list */
>>>
>>> v = visitor_input_test_init(data, "");
>>> @@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>>> visit_end_list(v, NULL);
>>> }
>>>
>>> +static void test_visitor_in_uintList(TestInputVisitorData *data,
>>> + const void *unused)
>>> +{
>>> + uint64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7, 8, 9, 1, 2, 3, 4, 5,
>>
>> Please wrap this line a bit earlier.
>
> Yes.
>
>>
>>> + 6, 7, 8 };
>>> + uint64_t expect2[] = { 32767, -32768, -32767 };
>>> + uint64_t expect3[] = { UINT64_MAX };
>>> + Error *err = NULL;
>>> + uint64List *res = NULL;
>>> + Visitor *v;
>>> + uint64_t val;
>>> +
>>> + /* Valid lists */
>>> +
>>> + v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>> + check_ulist(v, expect1, ARRAY_SIZE(expect1));
>>> +
>>> + v = visitor_input_test_init(data, "32767,-32768--32767");
>>> + check_ulist(v, expect2, ARRAY_SIZE(expect2));
>>> +
>>> + v = visitor_input_test_init(data, "18446744073709551615");
>>> + check_ulist(v, expect3, ARRAY_SIZE(expect3));
>>
>> Test behavior for large negative numbers?
>
> Yes, will add.
Again, a separate patch before PATCH 3 might be best.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-15 9:48 ` Markus Armbruster
@ 2018-11-15 10:16 ` David Hildenbrand
2018-11-15 14:57 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-15 10:16 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel, Michael Roth
On 15.11.18 10:48, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> On 14.11.18 18:38, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> The input visitor has some problems right now, especially
>>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>>> inconsistent behavior and ugly/magical code
>>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>>> supported and error messages are misleading
>>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>>> we should rather report an error
>>>> - lists/ranges are preparsed using int64_t, making it hard to
>>>> implement uint64_t values or uint64_t lists
>>>> - types that don't support lists don't bail out
>>>
>>> Known weirdness: empty list is invalid (test-string-input-visitor.c
>>> demonstates). Your patch doesn't change that (or else it would update
>>> the test). Should it be changed?
>>>
>>
>> I don't change the test, so the old behavior still works.
>> (empty string -> error)
>
> Understand. Design question: should it remain an error? Feel free to
> declare the question out of scope for this patch.
I think I was confused, let me retry to explain.
Empty lists actually don't result in an error. Calling start_list() on
an empty string works just fine.
However
- check_list() will result in "Fewer list elements expected"
- visit_type_.*int64() will result in "Fewer list elements expected"
- next_list() will result in NULL
I guess that is the intended behavior. E.g. the test does
v = visitor_input_test_init(data, "");
visit_type_uint64List(v, NULL, &res, &error_abort);
g_assert(!res);
So there won't be any error as the first "visit_next_list()" will
properly indicate "NULL".
>> Added "Only flat lists of integers (int64/uint64) are supported."
>
> Hmm, do lists of narrower integer types also work? I guess they do: the
> narrower visit_type_*int*() call v->type_*int64() via
> visit_type_*intN().
>
> Lists of type size are expressly excluded, in parse_type_size() below.
> That's okay, we can lift the restriction when it gets in the way.
Right, we can make that clearer
"Only flat lists of integers (except type "size") are supported." ?
[...]
>
>> What about "Less list elements expected"? That at least matches the
>> other error.
>
> Good enough. I'd say "fewer", though.
Fine with me!
[...]
>>>> + return;
>>>> + case LM_UNPARSED:
>>>> + if (try_parse_int64_list_entry(siv, obj)) {
>>>> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>>>> + "list of int64 values or ranges");
>>>> + }
>>>
>>> I figure I'd make try_parse_int64_list_entry() just parse, and on
>>> success fall through to case LM_INT64_RANGE. But your solution works,
>>> too.
>>
>> Then we would have to represent even single values as ranges, which is
>> something I'd like to avoid.
>
> Your artistic license applies.
It actually looks nicer your way (and seems to be less error prone).
Stay tuned!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-14 16:09 ` Markus Armbruster
@ 2018-11-15 11:09 ` David Hildenbrand
2018-11-15 13:17 ` Eric Blake
0 siblings, 1 reply; 20+ messages in thread
From: David Hildenbrand @ 2018-11-15 11:09 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel, Paolo Bonzini, Michael Roth
On 14.11.18 17:09, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
>
>> Let's use the new function.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> qapi/string-input-visitor.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b3fdd0827d..dee708d384 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -20,6 +20,7 @@
>> #include "qemu/option.h"
>> #include "qemu/queue.h"
>> #include "qemu/range.h"
>> +#include "qemu/cutils.h"
>>
>>
>> struct StringInputVisitor
>> @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
>> Error **errp)
>> {
>> StringInputVisitor *siv = to_siv(v);
>> - char *endp = (char *) siv->string;
>> double val;
>>
>> - errno = 0;
>> - val = strtod(siv->string, &endp);
>> - if (errno || endp == siv->string || *endp) {
>> + if (qemu_strtod(siv->string, NULL, &val)) {
>> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>> "number");
>> return;
>
> Three more: in qobject-input-visitor.c's
> qobject_input_type_number_keyval(),
This one is interesting, as it properly bails out when parsing "inf"
(via isFinite()). - should we do the same for the string input visitor?
Especially, should we forbid "inf" and "NaN" in both scenarios?
cutil.c's do_strtosz(), and
> json-parser.c's parse_literal().
>
> The latter doesn't check for errors since the lexer ensures the input is
> sane. Overflow can still happen, and is silently ignored. Feel free
> not to convert this one.
>
I'll do the conversion of all (allowing -ERANGE where it used to be
allow), we can then discuss with the patches at hand if it makes sense.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-15 11:09 ` David Hildenbrand
@ 2018-11-15 13:17 ` Eric Blake
2018-11-15 13:54 ` David Hildenbrand
2018-11-15 14:43 ` Markus Armbruster
0 siblings, 2 replies; 20+ messages in thread
From: Eric Blake @ 2018-11-15 13:17 UTC (permalink / raw)
To: David Hildenbrand, Markus Armbruster
Cc: Paolo Bonzini, qemu-devel, Michael Roth
On 11/15/18 5:09 AM, David Hildenbrand wrote:
>> Three more: in qobject-input-visitor.c's
>> qobject_input_type_number_keyval(),
>
> This one is interesting, as it properly bails out when parsing "inf"
> (via isFinite()). - should we do the same for the string input visitor?
>
> Especially, should we forbid "inf" and "NaN" in both scenarios?
JSON can't represent non-finite doubles. Internally, we might be able to
use them, but you have a point that consistently rejecting non-finite in
all of our QAPI parsers makes it easier to reason about the code base
(the command line can't be used to inject a value not possible via QMP).
So that makes sense to me. qemu_strtod() shouldn't reject non-finite
numbers (because it is useful for more than just qapi), but we could add
a new qemu_strtod_finite() if that would help avoid duplication.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-15 13:17 ` Eric Blake
@ 2018-11-15 13:54 ` David Hildenbrand
2018-11-15 14:43 ` Markus Armbruster
1 sibling, 0 replies; 20+ messages in thread
From: David Hildenbrand @ 2018-11-15 13:54 UTC (permalink / raw)
To: Eric Blake, Markus Armbruster; +Cc: Paolo Bonzini, qemu-devel, Michael Roth
On 15.11.18 14:17, Eric Blake wrote:
> On 11/15/18 5:09 AM, David Hildenbrand wrote:
>
>>> Three more: in qobject-input-visitor.c's
>>> qobject_input_type_number_keyval(),
>>
>> This one is interesting, as it properly bails out when parsing "inf"
>> (via isFinite()). - should we do the same for the string input visitor?
>>
>> Especially, should we forbid "inf" and "NaN" in both scenarios?
>
> JSON can't represent non-finite doubles. Internally, we might be able to
> use them, but you have a point that consistently rejecting non-finite in
> all of our QAPI parsers makes it easier to reason about the code base
> (the command line can't be used to inject a value not possible via QMP).
> So that makes sense to me. qemu_strtod() shouldn't reject non-finite
> numbers (because it is useful for more than just qapi), but we could add
> a new qemu_strtod_finite() if that would help avoid duplication.
>
Yes, I'll exactly add that! Thanks
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor
2018-11-15 13:17 ` Eric Blake
2018-11-15 13:54 ` David Hildenbrand
@ 2018-11-15 14:43 ` Markus Armbruster
1 sibling, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2018-11-15 14:43 UTC (permalink / raw)
To: Eric Blake; +Cc: David Hildenbrand, Paolo Bonzini, qemu-devel, Michael Roth
Eric Blake <eblake@redhat.com> writes:
> On 11/15/18 5:09 AM, David Hildenbrand wrote:
>
>>> Three more: in qobject-input-visitor.c's
>>> qobject_input_type_number_keyval(),
>>
>> This one is interesting, as it properly bails out when parsing "inf"
>> (via isFinite()). - should we do the same for the string input visitor?
>>
>> Especially, should we forbid "inf" and "NaN" in both scenarios?
>
> JSON can't represent non-finite doubles. Internally, we might be able
> to use them, but you have a point that consistently rejecting
> non-finite in all of our QAPI parsers makes it easier to reason about
> the code base (the command line can't be used to inject a value not
> possible via QMP). So that makes sense to me.
Given the liberties we already take with JSON in QAPI, "JSON can't do X"
need not immediately end a conversation about QAPI.
That said, consistency between QObject and string visitor is desirable.
The QObject input visitor comes in two flavours: one for use with the
JSON parser, and one for use with keyval_parse().
The latter flavour's qobject_input_type_number_keyval() rejects
non-finite numbers.
The former flavour's qobject_input_type_number() leaves rejecting "bad"
numbers to the JSON parser. The JSON parser doesn't accept special
syntax for NaN or infinity. It maps large numbers to HUGE_VAL with the
appropriate sign. Whether +/-HUGE_VAL are infinities depends on the
host. If we really want to outlaw infinities, we better fix
parse_literal() to check for ERANGE.
> qemu_strtod() shouldn't
> reject non-finite numbers (because it is useful for more than just
> qapi),
Yes.
> but we could add a new qemu_strtod_finite() if that would help
> avoid duplication.
Maybe.
qemu_strtod(str, NULL, &dbl) && isfinite(dbl)
isn't that much shorter or clearer than
qemu_strtod_finite(str, NULL, &dbl)
>> cutil.c's do_strtosz(), and
>> json-parser.c's parse_literal().
>>
>> The latter doesn't check for errors since the lexer ensures the input is
>> sane. Overflow can still happen, and is silently ignored. Feel free
>> not to convert this one.
>>
>
> I'll do the conversion of all (allowing -ERANGE where it used to be
> allow), we can then discuss with the patches at hand if it makes sense.
Sure!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor
2018-11-15 10:16 ` David Hildenbrand
@ 2018-11-15 14:57 ` Markus Armbruster
0 siblings, 0 replies; 20+ messages in thread
From: Markus Armbruster @ 2018-11-15 14:57 UTC (permalink / raw)
To: David Hildenbrand; +Cc: Paolo Bonzini, qemu-devel, Michael Roth
David Hildenbrand <david@redhat.com> writes:
> On 15.11.18 10:48, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> On 14.11.18 18:38, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> The input visitor has some problems right now, especially
>>>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>>>> inconsistent behavior and ugly/magical code
>>>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>>>> supported and error messages are misleading
>>>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>>>> we should rather report an error
>>>>> - lists/ranges are preparsed using int64_t, making it hard to
>>>>> implement uint64_t values or uint64_t lists
>>>>> - types that don't support lists don't bail out
>>>>
>>>> Known weirdness: empty list is invalid (test-string-input-visitor.c
>>>> demonstates). Your patch doesn't change that (or else it would update
>>>> the test). Should it be changed?
>>>>
>>>
>>> I don't change the test, so the old behavior still works.
>>> (empty string -> error)
>>
>> Understand. Design question: should it remain an error? Feel free to
>> declare the question out of scope for this patch.
>
> I think I was confused, let me retry to explain.
>
> Empty lists actually don't result in an error. Calling start_list() on
> an empty string works just fine.
>
> However
> - check_list() will result in "Fewer list elements expected"
> - visit_type_.*int64() will result in "Fewer list elements expected"
> - next_list() will result in NULL
>
> I guess that is the intended behavior. E.g. the test does
>
> v = visitor_input_test_init(data, "");
> visit_type_uint64List(v, NULL, &res, &error_abort);
> g_assert(!res);
>
> So there won't be any error as the first "visit_next_list()" will
> properly indicate "NULL".
You know, I was confused, too :) I looked at commit 3d089cea0d3, which
added the test case:
+ /* Empty list is invalid (weird) */
+
+ v = visitor_input_test_init(data, "");
+ visit_type_int64List(v, NULL, &res, &err);
+ error_free_or_abort(&err);
I missed regression fix commit d2788227c61:
- /* Empty list is invalid (weird) */
+ /* Empty list */
v = visitor_input_test_init(data, "");
- visit_type_int64List(v, NULL, &res, &err);
- error_free_or_abort(&err);
+ visit_type_int64List(v, NULL, &res, &error_abort);
+ g_assert(!res);
So the test actually demonstrates empty lists work fine before and after
your patch.
>>> Added "Only flat lists of integers (int64/uint64) are supported."
>>
>> Hmm, do lists of narrower integer types also work? I guess they do: the
>> narrower visit_type_*int*() call v->type_*int64() via
>> visit_type_*intN().
>>
>> Lists of type size are expressly excluded, in parse_type_size() below.
>> That's okay, we can lift the restriction when it gets in the way.
>
> Right, we can make that clearer
>
> "Only flat lists of integers (except type "size") are supported." ?
> [...]
Sold!
[...]
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-11-15 14:57 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09 11:02 [Qemu-devel] [PATCH RFC 0/6] qapi: rewrite string-input-visitor David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 1/6] cutils: add qemu_strtod() David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 2/6] qapi: use qemu_strtod() in string-input-visitor David Hildenbrand
2018-11-14 16:09 ` Markus Armbruster
2018-11-15 11:09 ` David Hildenbrand
2018-11-15 13:17 ` Eric Blake
2018-11-15 13:54 ` David Hildenbrand
2018-11-15 14:43 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor David Hildenbrand
2018-11-14 17:38 ` Markus Armbruster
2018-11-14 19:56 ` David Hildenbrand
2018-11-15 9:48 ` Markus Armbruster
2018-11-15 10:16 ` David Hildenbrand
2018-11-15 14:57 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 4/6] test-string-input-visitor: use virtual walk David Hildenbrand
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 5/6] test-string-input-visitor: split off uint64 list tests David Hildenbrand
2018-11-14 16:21 ` Markus Armbruster
2018-11-14 20:03 ` David Hildenbrand
2018-11-15 9:59 ` Markus Armbruster
2018-11-09 11:02 ` [Qemu-devel] [PATCH RFC 6/6] test-string-input-visitor: add range overflow tests David Hildenbrand
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.