Re: [Qemu-devel] [PATCH v11 23/28] qapi: Fix alternates that accept 'number' but not 'int'

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The QMP input visitor allows integral values to be assigned by
> promotion to a QTYPE_QFLOAT.  However, when parsing an alternate,
> we did not take this into account, such that an alternate that
> accepts 'number' and some other type, but not 'int', would reject
> integral values.
>
> With this patch, we now have the following desirable table:
>
>     alternate has      case selected for
>     'int'  'number'    QTYPE_QINT  QTYPE_QFLOAT
>       no        no     error       error
>       no       yes     'number'    'number'
>      yes        no     'int'       error
>      yes       yes     'int'       'number'
>
> While it is unlikely that we will ever use 'number' in an
> alternate other than in the testsuite, it never hurts to be
> more precise in what we allow.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v11 (no v10): slight commit message tweak, rebase to earlier changes
> v9: rebase to earlier changes
> v8: no change
> v7: rebase to named .u union
> v6: rebase onto earlier testsuite and gen_err_check() improvements
> ---
>  include/qapi/visitor-impl.h    |  2 +-
>  include/qapi/visitor.h         |  3 ++-
>  qapi/qapi-visit-core.c         |  4 ++--
>  qapi/qmp-input-visitor.c       |  4 ++++
>  scripts/qapi-visit.py          | 11 +++++++----
>  tests/test-qmp-input-visitor.c | 16 ++++++----------
>  6 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
> index e36da60..ac6c17e 100644
> --- a/include/qapi/visitor-impl.h
> +++ b/include/qapi/visitor-impl.h
> @@ -33,7 +33,7 @@ struct Visitor
>      void (*type_enum)(Visitor *v, int *obj, const char * const strings[],
>                        const char *kind, const char *name, Error **errp);
>      /* May be NULL; only needed for input visitors. */
> -    void (*get_next_type)(Visitor *v, QTypeCode *type,
> +    void (*get_next_type)(Visitor *v, QTypeCode *type, bool promote_int,
>                            const char *name, Error **errp);
>
>      void (*type_int)(Visitor *v, int64_t *obj, const char *name, Error 
> **errp);
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 4fa6d50..250e8e1 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -46,8 +46,9 @@ void visit_optional(Visitor *v, bool *present, const char 
> *name,
>   * Determine the qtype of the item @name in the current object visit.
>   * For input visitors, set address@hidden to the correct qtype of a qapi
>   * alternate type; for other visitors, leave address@hidden unchanged.
> + * If @promote_int, treat integers as QTYPE_FLOAT.
>   */
> -void visit_get_next_type(Visitor *v, QTypeCode *type,
> +void visit_get_next_type(Visitor *v, QTypeCode *type, bool promote_int,
>                           const char *name, Error **errp);
>  void visit_type_enum(Visitor *v, int *obj, const char * const strings[],
>                       const char *kind, const char *name, Error **errp);
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index ddb3a15..52c132a 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -81,11 +81,11 @@ void visit_optional(Visitor *v, bool *present, const char 
> *name,
>      }
>  }
>
> -void visit_get_next_type(Visitor *v, QTypeCode *type,
> +void visit_get_next_type(Visitor *v, QTypeCode *type, bool promote_int,
>                           const char *name, Error **errp)
>  {
>      if (v->get_next_type) {
> -        v->get_next_type(v, type, name, errp);
> +        v->get_next_type(v, type, promote_int, name, errp);
>      }
>  }
>
> diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c
> index 2141ff5..4238faf 100644
> --- a/qapi/qmp-input-visitor.c
> +++ b/qapi/qmp-input-visitor.c
> @@ -209,6 +209,7 @@ static void qmp_input_end_list(Visitor *v, Error **errp)
>  }
>
>  static void qmp_input_get_next_type(Visitor *v, QTypeCode *type,
> +                                    bool promote_int,
>                                      const char *name, Error **errp)
>  {
>      QmpInputVisitor *qiv = to_qiv(v);
> @@ -219,6 +220,9 @@ static void qmp_input_get_next_type(Visitor *v, QTypeCode 
> *type,
>          return;
>      }
>      *type = qobject_type(qobj);
> +    if (promote_int && *type == QTYPE_QINT) {
> +        *type = QTYPE_QFLOAT;
> +    }
>  }
>

In case you also wonder why only the QMP input visitor implements this
method: the generated alternate visitors are the only users of
visit_get_next_type(), and so far alternates are only used with QMP.  I
think.

>  static void qmp_input_type_int(Visitor *v, int64_t *obj, const char *name,
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index ad98685..4c9d739 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -183,6 +183,11 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, 
> const char *name, Error
>
>
>  def gen_visit_alternate(name, variants):
> +    promote_int = 'true'
> +    for var in variants.variants:
> +        if var.type.alternate_qtype() == 'QTYPE_QINT':
> +            promote_int = 'false'
> +
>      ret = mcgen('''
>
>  void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, 
> Error **errp)
> @@ -193,16 +198,14 @@ void visit_type_%(c_name)s(Visitor *v, %(c_name)s 
> **obj, const char *name, Error
>      if (err) {
>          goto out;
>      }
> -    visit_get_next_type(v, &(*obj)->type, name, &err);
> +    visit_get_next_type(v, &(*obj)->type, %(promote_int)s, name, &err);
>      if (err) {
>          goto out_obj;
>      }
>      switch ((*obj)->type) {
>  ''',
> -                c_name=c_name(name))
> +                c_name=c_name(name), promote_int=promote_int)
>
> -    # FIXME: When 'number' but not 'int' is present in the alternate, we
> -    # should allow QTYPE_INT to promote to QTYPE_FLOAT.
>      for var in variants.variants:
>          ret += mcgen('''
>      case %(case)s:
> diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
> index 43b9e18..b4a5bee 100644
> --- a/tests/test-qmp-input-visitor.c
> +++ b/tests/test-qmp-input-visitor.c
> @@ -347,20 +347,16 @@ static void 
> test_visitor_in_alternate_number(TestInputVisitorData *data,
>      error_free_or_abort(&err);
>      qapi_free_AltStrBool(asb);
>
> -    /* FIXME: integer should parse as number */
>      v = visitor_input_test_init(data, "42");
> -    visit_type_AltStrNum(v, &asn, NULL, &err);
> -    /* FIXME g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT); */
> -    /* FIXME g_assert_cmpfloat(asn->u.n, ==, 42); */
> -    error_free_or_abort(&err);
> +    visit_type_AltStrNum(v, &asn, NULL, &error_abort);
> +    g_assert_cmpint(asn->type, ==, QTYPE_QFLOAT);
> +    g_assert_cmpfloat(asn->u.n, ==, 42);
>      qapi_free_AltStrNum(asn);
>
> -    /* FIXME: integer should parse as number */
>      v = visitor_input_test_init(data, "42");
> -    visit_type_AltNumStr(v, &ans, NULL, &err);
> -    /* FIXME g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT); */
> -    /* FIXME g_assert_cmpfloat(ans->u.n, ==, 42); */
> -    error_free_or_abort(&err);
> +    visit_type_AltNumStr(v, &ans, NULL, &error_abort);
> +    g_assert_cmpint(ans->type, ==, QTYPE_QFLOAT);
> +    g_assert_cmpfloat(ans->u.n, ==, 42);
>      qapi_free_AltNumStr(ans);
>
>      v = visitor_input_test_init(data, "42");

Looks good.