Re: [Qemu-devel] [PATCH v18 11/14] qapi: make string input visitor parse int list

[Hu Tao]

On Wed, Feb 19, 2014 at 03:54:02PM +0800, Hu Tao wrote:
> Cc: Laszlo Ersek <address@hidden>
> Signed-off-by: Hu Tao <address@hidden>
> ---
>  qapi/string-input-visitor.c       | 160 
> ++++++++++++++++++++++++++++++++++++--
>  tests/test-string-input-visitor.c |  22 ++++++
>  2 files changed, 176 insertions(+), 6 deletions(-)
> 
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index a152f5d..4540ca3 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -15,30 +15,175 @@
>  #include "qapi/visitor-impl.h"
>  #include "qapi/qmp/qerror.h"
>  
> +enum ListMode {
> +    LM_NONE,             /* not traversing a list of repeated options */
> +    LM_STARTED,          /* start_list() succeeded */
> +
> +    LM_IN_PROGRESS,      /* next_list() has been called.
> +                          *
> +                          * Generating the next list link will consume the 
> most
> +                          * recently parsed QemuOpt instance of the repeated
> +                          * option.
> +                          *
> +                          * Parsing a value into the list link will examine 
> the
> +                          * next QemuOpt instance of the repeated option, and
> +                          * possibly enter LM_SIGNED_INTERVAL or
> +                          * LM_UNSIGNED_INTERVAL.
> +                          */
> +
> +    LM_SIGNED_INTERVAL,  /* next_list() has been called.
> +                          *
> +                          * Generating the next list link will consume the 
> most
> +                          * recently stored element from the signed interval,
> +                          * parsed from the most recent QemuOpt instance of 
> the
> +                          * repeated option. This may consume QemuOpt itself
> +                          * and return to LM_IN_PROGRESS.
> +                          *
> +                          * Parsing a value into the list link will store the
> +                          * next element of the signed interval.
> +                          */
> +
> +    LM_UNSIGNED_INTERVAL,/* Same as above, only for an unsigned interval. */
> +
> +    LM_END
> +};
> +
> +typedef enum ListMode ListMode;
> +
>  struct StringInputVisitor
>  {
>      Visitor visitor;
> +
> +    ListMode list_mode;
> +
> +    /* When parsing a list of repeating options as integers, values of the 
> form
> +     * "a-b", representing a closed interval, are allowed. Elements in the
> +     * range are generated individually.
> +     */
> +    union {
> +        int64_t s;
> +        uint64_t u;
> +    } range_next, range_limit;
> +
>      const char *string;
>  };
>  
> +static void
> +start_list(Visitor *v, const char *name, Error **errp)
> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +
> +    /* we can't traverse a list in a list */
> +    assert(siv->list_mode == LM_NONE);
> +    siv->list_mode = LM_STARTED;
> +}
> +
> +static GenericList *
> +next_list(Visitor *v, GenericList **list, Error **errp)
> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +    GenericList **link;
> +
> +    switch (siv->list_mode) {
> +    case LM_STARTED:
> +        siv->list_mode = LM_IN_PROGRESS;
> +        link = list;
> +        break;
> +
> +    case LM_SIGNED_INTERVAL:
> +    case LM_UNSIGNED_INTERVAL:
> +        link = &(*list)->next;
> +
> +        if (siv->list_mode == LM_SIGNED_INTERVAL) {
> +            if (siv->range_next.s < siv->range_limit.s) {
> +                ++siv->range_next.s;
> +                break;
> +            }
> +        } else if (siv->range_next.u < siv->range_limit.u) {
> +            ++siv->range_next.u;
> +            break;
> +        }
> +        siv->list_mode = LM_END;
> +        /* range has been completed, fall through */
> +
> +    case LM_END:
> +        return NULL;
> +
> +    case LM_IN_PROGRESS:
> +        link = &(*list)->next;
> +        break;
> +
> +    default:
> +        abort();
> +    }
> +
> +    *link = g_malloc0(sizeof **link);
> +    return *link;
> +}
> +
> +static void
> +end_list(Visitor *v, Error **errp)
> +{
> +    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> +
> +    assert(siv->list_mode == LM_STARTED ||
> +           siv->list_mode == LM_END ||
> +           siv->list_mode == LM_IN_PROGRESS ||
> +           siv->list_mode == LM_SIGNED_INTERVAL ||
> +           siv->list_mode == LM_UNSIGNED_INTERVAL);
> +    siv->list_mode = LM_NONE;
> +}
> +
>  static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
>                             Error **errp)
>  {
>      StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
> -    char *endp = (char *) siv->string;
> +    char *str = (char *) siv->string;
>      long long val;
> +    char *endptr;
>  
> -    errno = 0;
> -    if (siv->string) {
> -        val = strtoll(siv->string, &endp, 0);
> +    if (siv->list_mode == LM_SIGNED_INTERVAL) {
> +        *obj = siv->range_next.s;
> +        return;
>      }
> -    if (!siv->string || errno || endp == siv->string || *endp) {
> +
> +    if (!siv->string) {
>          error_set(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                    "integer");
>          return;
>      }
>  
> -    *obj = val;
> +    errno = 0;
> +    val = strtoll(siv->string, &endptr, 0);
> +
> +    if (errno == 0 && endptr > str && INT64_MIN <= val && val <= INT64_MAX) {
> +        if (*endptr == '\0') {
> +            *obj = val;
> +            siv->list_mode = LM_END;
> +            return;
> +        }
> +        if (*endptr == '-' && siv->list_mode == LM_IN_PROGRESS) {
> +            long long val2;
> +
> +            str = endptr + 1;
> +            val2 = strtoll(str, &endptr, 0);
> +            if (errno == 0 && endptr > str && *endptr == '\0' &&
> +                INT64_MIN <= val2 && val2 <= INT64_MAX && val <= val2 &&
> +                (val > INT64_MAX - 65536 ||
> +                 val2 < val + 65536)) {
> +                siv->range_next.s = val;
> +                siv->range_limit.s = val2;
> +                siv->list_mode = LM_SIGNED_INTERVAL;
> +
> +                /* as if entering on the top */
> +                *obj = siv->range_next.s;
> +                return;
> +            }
> +        }
> +    }
> +    error_set(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +              (siv->list_mode == LM_NONE) ? "an int64 value" :
> +                                           "an int64 value or range");
>  }

Two problems:

1. the code is mostly copied from OptsVisitor. maybe we can share the
   code?

2. int list is not implemented in string outout visitor. but there is
   currently no user of it. Should we implement it or not?