[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor |
Date: |
Tue, 20 Nov 2018 11:40:04 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
On 11/20/18 3:25 AM, David Hildenbrand wrote:
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
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted,
and duplicates eliminated
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.
Please note that no users/callers have to be fixed up. Candiates using
s/Candiates/Candidates/
visit_type_uint16List() and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes()
-- Code can deal with dupilcates/unsorted lists
s/dupilcates/duplicates/
- numa.c::query_memdev()
-- via object_property_get_uint16List(), the list will still be sorted
and without duplicates (via host_memory_backend_get_host_nodes())
- qapi-visit.c::visit_type_Memdev_members()
- qapi-visit.c::visit_type_NumaNodeOptions_members()
- qapi-visit.c::visit_type_RockerOfDpaGroup_members
- qapi-visit.c::visit_type_RxFilterInfo_members()
-- Not used with string-input-visitor.
Signed-off-by: David Hildenbrand <address@hidden>
---
include/qapi/string-input-visitor.h | 4 +-
qapi/string-input-visitor.c | 405 ++++++++++++++++------------
tests/test-string-input-visitor.c | 18 +-
3 files changed, 234 insertions(+), 193 deletions(-)
struct StringInputVisitor
{
Visitor visitor;
- GList *ranges;
- GList *cur_range;
- int64_t cur;
+ /* List parsing state */
+ ListMode lm;
+ RangeElement rangeNext;
+ RangeElement rangeEnd;
+ const char *unparsed_string;
+ void *list;
+ /* The original string to parse */
const char *string;
- void *list; /* Only needed for sanity checking the caller */
};
Makes sense.
@@ -179,88 +106,208 @@ 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_INT64_RANGE:
+ case LM_UINT64_RANGE:
+ case LM_UNPARSED:
+ error_setg(errp, "Fewer list elements expected");
Bike-shedding - I don't know if "Too many list elements supplied" would
make the error any more legible.
static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
Error **errp)
{
StringInputVisitor *siv = to_siv(v);
-
- if (parse_str(siv, name, errp) < 0) {
+ 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;
+ }
+ *obj = val;
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");
The error message might be a bit misleading for a range larger than 64k,
but that's not too bad.
+ return;
+ }
+ assert(siv->lm == LM_INT64_RANGE);
+ /* fall through */
+ case LM_INT64_RANGE:
+ /* return the next element in the range */
+ assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
+ *obj = siv->rangeNext.i64++;
+
+ if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
I think our compiler options guarantee that we have sane signed
wraparound and thus this is a safe comparison on overflow; but if you
were to swap it so that the *obj == INT64_MAX check is performed first,
you wouldn't even have to debate about whether undefined C semantics are
being invoked.
+ /* end of range, check if there is more to parse */
+ siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
+ }
+ return;
+ case LM_END:
+ error_setg(errp, "Fewer list elements expected");
Again, bikeshedding if "too many list elements supplied" would make any
more sense.
+static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
+{
+ const char *endptr;
+ uint64_t start, end;
- siv->cur_range = g_list_first(siv->ranges);
- if (!siv->cur_range) {
- goto error;
+ /* parse a simple uint64 or range */
+ if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
Lots of duplication between the signed and unsigned variants. But I
don't see any easy way to factor it out into a common helper, as there
are just too many places where signed vs. unsigned does not easily lend
itself to common code.
@@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char *name,
QNull **obj,
{
StringInputVisitor *siv = to_siv(v);
+ assert(siv->lm == LM_NONE);
*obj = NULL;
- if (!siv->string || siv->string[0]) {
+ if (siv->string[0]) {
Why did this condition change?
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
- [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, (continued)
- [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, David Hildenbrand, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Eric Blake, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Markus Armbruster, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Eric Blake, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, David Hildenbrand, 2018/11/21
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Markus Armbruster, 2018/11/21
- Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes, Eric Blake, 2018/11/21
[Qemu-devel] [PATCH v2 7/9] test-string-input-visitor: Use virtual walk, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor, David Hildenbrand, 2018/11/20
- Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor,
Eric Blake <=
[Qemu-devel] [PATCH v2 9/9] test-string-input-visitor: Add range overflow tests, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 8/9] test-string-input-visitor: Split off uint64 list tests, David Hildenbrand, 2018/11/20