qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v14 11/21] qapi: add integer range support for QObjectInputVisitor
Date: Wed, 12 Oct 2016 17:50:41 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

"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(-)
>
> 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(&qiv->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 = 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(&qiv->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++;

> +            return;
> +        } else {
> +            inlist = tos->entry != NULL;
> +        }
> +    }

I'd prefer

       tos = QSLIST_FIRST(&qiv->stack);
       inlist = tos && tos->entry;
       if (inlist) {
           if ((int64_t)tos->range_val < (int64_t)tos->range_limit) {
               *obj = tos->range_val++;
               return;
           }
       }

Note for later:

    !tos || (int64_t)tos->range_val >= (int64_t)tos->range_limit

holds here.

>  
> +    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 (qemu_strtoll(qstr->string, NULL, 0, &ret) < 0) {
> +    if (qemu_strtoll(qstr->string, &end, 0, &ret) < 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;
> +        }

Note for later: since inlist is tos && tos->entry, we know tos is
non-null here.  Recommend assert(tos) to make it more obvious.

> +        if (*end != '-') {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
> +                       "a number range");

"an integer or integer range", actually.

> +            return;
> +        }
> +        end++;
> +        if (qemu_strtoll(end, NULL, 0, &ret) < 0) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number");

"an integer".

> +            return;
> +        }
> +        if (*obj > ret) {
> +            error_setg(errp,
> +                       "Parameter '%s' range start %" PRIu64
> +                       " must be less than (or equal to) %" PRIu64,
> +                       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;
> +        }

I see you improved the error messages over the option visitor's.  Good.

> +        if (*obj != ret) {
> +            tos->range_val = *obj;
> +            tos->range_limit = ret;
> +        }

Does this need to be conditional?

Dereferencing tos is safe (see note above).

> +    }

No else, thus tos->range_val and tos->range_limit remain unchanged.
Works because we know that !tos || tos->range_val >= tos->range_limit
(see note above).  If it wasn't, the next visit wouldn't consume another
object as it should.  Considering the distance to the place that ensures
this, I'd prefer to spell this out here with an assertion, or perhaps by
assigning to tos->range_val and tos->range_limit.

>  }

Overall, less hairy than the options visitor.

>  
>  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?

>  
>  static void qobject_input_type_bool(Visitor *v, const char *name, bool *obj,
> @@ -576,7 +712,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)
>  {
>      QObjectInputVisitor *v;
>  
> @@ -603,6 +740,7 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>      v->strict = true;
>      v->autocreate_list = autocreate_list;
>      v->autocreate_struct_levels = autocreate_struct_levels;
> +    v->permit_int_ranges = permit_int_ranges;
>  
>      v->root = obj;
>      qobject_incref(obj);
> @@ -614,6 +752,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)
>  {
>      QDict *pdict;
> @@ -632,7 +771,8 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>  
>      v = qobject_input_visitor_new_autocast(pobj,
>                                             autocreate_list,
> -                                           autocreate_struct_levels);
> +                                           autocreate_struct_levels,
> +                                           permit_int_ranges);
>   cleanup:
>      qobject_decref(pobj);
>      QDECREF(pdict);
[Skipping test updates for now...]



reply via email to

[Prev in Thread] Current Thread [Next in Thread]