Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor

[Markus Armbruster <armbru@redhat.com>]

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!