Re: [Qemu-devel] [PATCH v14 11/21] qapi: add integer range support for QObjectInputVisitor

[Eric Blake]

On 09/30/2016 09:45 AM, Daniel P. Berrange wrote:
> The traditional CLI arg syntax allows two ways to specify
> integer lists, either one value per key, or a range of
> values per key. eg the following are identical:
> 
>   -arg foo=5,foo=6,foo=7
>   -arg foo=5-7
> 
> This extends the QObjectInputVisitor so that it is able
> to parse ranges and turn them into distinct list entries.
> 
> This means that
> 
>   -arg foo=5-7
> 
> is treated as equivalent to
> 
>   -arg foo.0=5,foo.1=6,foo.2=7
> 
> Edge case tests are copied from test-opts-visitor to
> ensure identical behaviour when parsing.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> @@ -71,6 +77,19 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   * The value given determines how many levels of structs are allowed to
>   * be flattened in this way.
>   *
> + * If @permit_int_ranges is true, then when visiting a list of integers,
> + * the integer value strings may encode ranges eg a single element

My usual complaint that 'e.g.' is usually spelled with dots :)

> + * containing "5-7" is treated as if there were three elements "5", "6",
> + * "7". This should only be used if compatibility is required with the
> + * OptsVisitor which would allow integer ranges. e.g. set this to true

Ah. Here you DID use dots, but used the wrong abbreviation.
Substituting "For example" in this location does not make as much sense
as "i.e." would.  Or go for simplicity; it reads just fine as:

"...would allow integer ranges; so only set this to true if..."

> + * if you have compatibility requirements for
> + *
> + *   -arg val=5-8
> + *
> + * to be treated as equivalent to the preferred syntax:
> + *
> + *   -arg val.0=5,val.1=6,val.2=7,val.3=8
> + *

> +
> +    /*
> +     * If we have string that represents an integer range (5-24),
> +     * parse the end of the range and set things up so we'll process
> +     * the rest of the range before consuming another element
> +     * from the QList.
> +     */
> +    if (end && *end) {
> +        if (!qiv->permit_int_ranges) {
> +            error_setg(errp,
> +                       "Integer ranges are not permitted here");
> +            return;
> +        }
> +        if (!inlist) {
> +            error_setg(errp,
> +                       "Integer ranges are only permitted when "
> +                       "visiting list parameters");
> +            return;
> +        }
> +        if (*end != '-') {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +                       "a number range");
> +            return;
> +        }
> +        end++;
> +        if (qemu_strtoll(end, NULL, 0, &ret) < 0) {

Signed parse...

> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
> +            return;
> +        }
> +        if (*obj > ret) {

...and signed comparison...

> +            error_setg(errp,
> +                       "Parameter '%s' range start %" PRIu64
> +                       " must be less than (or equal to) %" PRIu64,

...but unsigned numbers in the error message.  Looks a bit odd (but not
necessarily worse than what our OptsVisitor was already doing).

> +                       name, *obj, ret);
> +            return;
> +        }
> +
> +        if ((*obj <= (INT64_MAX - QIV_RANGE_MAX)) &&
> +            (ret >= (*obj + QIV_RANGE_MAX))) {
> +            error_setg(errp,
> +                       "Parameter '%s' range must be less than %d",
> +                       name, QIV_RANGE_MAX);
> +            return;
> +        }
> +        if (*obj != ret) {
> +            tos->range_val = *obj;
> +            tos->range_limit = ret;
> +        }
> +    }
>  }

Overall, I found this a bit easier to read than the OptsVisitor state
machine.  Which makes me worried that we may have some subtle
translation bugs; but I hope it's the same end result.

My consolation is that if there are any differences, they would be in
the corner cases, where a user is probably going to get surprising
behavior from the command line anyway, and shouldn't have been
requesting ranges in that corner case.

>  
>  static void qobject_input_type_uint64(Visitor *v, const char *name,
> @@ -366,21 +438,85 @@ static void qobject_input_type_uint64_autocast(Visitor 
> *v, const char *name,
>                                                 uint64_t *obj, Error **errp)
>  {
>      QObjectInputVisitor *qiv = to_qiv(v);

Feels like a lot of code duplication; but I don't know if there's any
sane way to share more of the code while handling both signed and
unsigned types.

The testsuite additions are reassuring; I confirmed that they match
test-opts-visitor.c in coverage.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature