From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56364) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwtNk-00035S-6B for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:01:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwtNg-0006C0-Pn for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:01:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57165) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwtNg-0006Bo-Gg for qemu-devel@nongnu.org; Thu, 12 Nov 2015 10:01:44 -0500 From: Markus Armbruster References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-24-git-send-email-eblake@redhat.com> Date: Thu, 12 Nov 2015 16:01:41 +0100 In-Reply-To: <1447224690-9743-24-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 10 Nov 2015 23:51:25 -0700") Message-ID: <87fv0bjm7e.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v11 23/28] qapi: Fix alternates that accept 'number' but not 'int' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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 > > --- > 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.