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

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> On Wed, Oct 12, 2016 at 05:50:41PM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange"  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 
>> > ---
>> >  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(>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, , 0) < 0) {
>> > +if (parse_uint(qstr->string, , , 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, , 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

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"  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 
> > ---
> >  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(>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, , 0) < 0) {
> > +if (parse_uint(qstr->string, , , 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, , 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 :|
|: 

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

[Markus Armbruster]

Kevin Wolf  writes:

> Am 12.10.2016 um 17:50 hat Markus Armbruster geschrieben:
>> "Daniel P. Berrange"  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 
>
>> > @@ -329,21 +335,87 @@ static void 
>> > qobject_input_type_int64_autocast(Visitor *v, const char *name,
>> >int64_t *obj, Error **errp)
>> >  {
>> >  QObjectInputVisitor *qiv = to_qiv(v);
>> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
>> > -true));
>> > +QString *qstr;
>> >  int64_t ret;
>> > +const char *end = NULL;
>> > +StackObject *tos;
>> > +bool inlist = false;
>> > +
>> > +/* Preferentially generate values from a range, before
>> > + * trying to consume another QList element */
>> > +tos = QSLIST_FIRST(>stack);
>> > +if (tos) {
>> > +if ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
>> > +*obj = tos->range_val + 1;
>> > +tos->range_val++;
>> 
>> Roundabout way to write
>> 
>>*obj = tos->range_val++;
>
> *obj = ++tos->range_val, actually.

Of course, thanks.

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

[Kevin Wolf]

Am 12.10.2016 um 17:50 hat Markus Armbruster geschrieben:
> "Daniel P. Berrange"  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 

> > @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> > *v, const char *name,
> >int64_t *obj, Error **errp)
> >  {
> >  QObjectInputVisitor *qiv = to_qiv(v);
> > -QString *qstr = qobject_to_qstring(qobject_input_get_object(qiv, name,
> > -true));
> > +QString *qstr;
> >  int64_t ret;
> > +const char *end = NULL;
> > +StackObject *tos;
> > +bool inlist = false;
> > +
> > +/* Preferentially generate values from a range, before
> > + * trying to consume another QList element */
> > +tos = QSLIST_FIRST(>stack);
> > +if (tos) {
> > +if ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
> > +*obj = tos->range_val + 1;
> > +tos->range_val++;
> 
> Roundabout way to write
> 
>*obj = tos->range_val++;

*obj = ++tos->range_val, actually.

Kevin

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

[Markus Armbruster]

"Daniel P. Berrange"  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 
> ---
>  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(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 94051f3..242b767 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -19,6 +19,12 @@
>  
>  typedef struct QObjectInputVisitor QObjectInputVisitor;
>  
> +/* Inclusive upper bound on the size of any flattened range. This is a safety
> + * (= anti-annoyance) measure; wrong ranges should not cause long startup
> + * delays nor exhaust virtual memory.
> + */
> +#define QIV_RANGE_MAX 65536
> +
>  /**
>   * Create a new input visitor that converts @obj to a QAPI object.
>   *
> @@ -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
> + * 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
> + * 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
> + *
>   * The visitor always operates in strict mode, requiring all dict keys
>   * to be consumed during visitation. An error will be reported if this
>   * does not happen.
> @@ -80,7 +99,8 @@ Visitor *qobject_input_visitor_new(QObject *obj, bool 
> strict);
>   */
>  Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  bool autocreate_list,
> -size_t autocreate_struct_levels);
> +size_t autocreate_struct_levels,
> +bool permit_int_ranges);
>  
>  
>  /**
> @@ -98,6 +118,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>  Visitor *qobject_input_visitor_new_opts(const QemuOpts *opts,
>  bool autocreate_list,
>  size_t autocreate_struct_levels,
> +bool permit_int_ranges,
>  Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 1be4865..6b3d0f2 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -31,6 +31,8 @@ typedef struct StackObject
>  
>  GHashTable *h;   /* If obj is dict: unvisited keys */
>  const QListEntry *entry; /* If obj is list: unvisited tail */
> +uint64_t range_val;
> +uint64_t range_limit;

It's either ugly unions or ugly type casts.  The options visitor picked
ugly unions, you're picking ugly type casts.  Matter of taste, as long
as the type casts are all kosher.

>  
>  QSLIST_ENTRY(StackObject) node;
>  } StackObject;
> @@ -60,6 +62,10 @@ struct QObjectInputVisitor
>   * consider auto-creating a struct containing
>   * remaining unvisited items */
>  size_t autocreate_struct_levels;
> +
> +/* Whether int lists can have single values representing
> + * ranges of values */
> +bool permit_int_ranges;
>  };
>  
>  static QObjectInputVisitor *to_qiv(Visitor *v)
> @@ -282,7 +288,7 @@ static GenericList *qobject_input_next_list(Visitor *v, 
> GenericList *tail,
>  QObjectInputVisitor *qiv = to_qiv(v);
>  StackObject *so = QSLIST_FIRST(>stack);
>  
> -if (!so->entry) {
> +if ((so->range_val == so->range_limit) && !so->entry) {
>  return NULL;
>  }
>  tail->next = g_malloc0(size);
> @@ -329,21 +335,87 @@ static void qobject_input_type_int64_autocast(Visitor 
> *v, const char *name,
>int64_t *obj, Error **errp)
>  {
>  QObjectInputVisitor *qiv =