Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > > --- > 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 *@type to the correct qtype of a qapi > * alternate type; for other visitors, leave *@type 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.