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

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Wed, Oct 12, 2016 at 05:50:41PM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" <address@hidden> writes:
>> 
>> > 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>
>> > ---
>> >  include/qapi/qobject-input-visitor.h |  23 ++++-
>> >  qapi/qobject-input-visitor.c         | 158 ++++++++++++++++++++++++++--
>> >  tests/test-qobject-input-visitor.c   | 195 
>> > +++++++++++++++++++++++++++++++++--
>> >  3 files changed, 360 insertions(+), 16 deletions(-)
>> >
>
>> >  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);
>> > -    QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> > -                                                                true));
>> > +    QString *qstr;
>> >      unsigned long long ret;
>> > +    char *end = NULL;
>> > +    StackObject *tos;
>> > +    bool inlist = false;
>> > +
>> > +    /* Preferentially generate values from a range, before
>> > +     * trying to consume another QList element */
>> > +    tos = QSLIST_FIRST(&qiv->stack);
>> > +    if (tos) {
>> > +        if (tos->range_val < tos->range_limit) {
>> > +            *obj = tos->range_val + 1;
>> > +            tos->range_val++;
>> > +            return;
>> > +        } else {
>> > +            inlist = tos->entry != NULL;
>> > +        }
>> > +    }
>> >  
>> > +    qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> > +                                                       true));
>> >      if (!qstr || !qstr->string) {
>> >          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : 
>> > "null",
>> >                     "string");
>> >          return;
>> >      }
>> >  
>> > -    if (parse_uint_full(qstr->string, &ret, 0) < 0) {
>> > +    if (parse_uint(qstr->string, &ret, &end, 0) < 0) {
>> >          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");
>> >          return;
>> >      }
>> >      *obj = ret;
>> > +
>> > +    /*
>> > +     * 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 (parse_uint_full(end, &ret, 0) < 0) {
>> > +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a 
>> > number");
>> > +            return;
>> > +        }
>> > +        if (*obj > ret) {
>> > +            error_setg(errp,
>> > +                       "Parameter '%s' range start %" PRIu64
>> > +                       " must be less than (or equal to) %llu",
>> > +                       name, *obj, ret);
>> > +            return;
>> > +        }
>> > +        if ((ret - *obj) > (QIV_RANGE_MAX - 1)) {
>> > +            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;
>> > +        }
>> > +    }
>> >  }
>> 
>> Duplicates the signed code, which is sad, but I don't have better ideas.
>> 
>> Except this one: are we actually using both the signed and the unsigned
>> case now?  If not, can we get rid of the one we don't use?
>
> Out of the args that I converted in this series, I only see uint16List
> used. eg for -numa and -object hostmem

Let's drop the signed copy then.