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

[Daniel P. Berrange]

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

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|