From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46081) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buKsr-000233-Ca for qemu-devel@nongnu.org; Wed, 12 Oct 2016 10:51:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buKsq-0004or-32 for qemu-devel@nongnu.org; Wed, 12 Oct 2016 10:51:53 -0400 From: Markus Armbruster References: <1475246744-29302-1-git-send-email-berrange@redhat.com> <1475246744-29302-8-git-send-email-berrange@redhat.com> <87d1j6amdm.fsf@dusky.pond.sub.org> Date: Wed, 12 Oct 2016 16:51:43 +0200 In-Reply-To: <87d1j6amdm.fsf@dusky.pond.sub.org> (Markus Armbruster's message of "Tue, 11 Oct 2016 18:20:05 +0200") Message-ID: <87bmyptybk.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v14 07/21] qapi: permit scalar type conversions in QObjectInputVisitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Paolo Bonzini , Andreas =?utf-8?Q?F=C3=A4rber?= Markus Armbruster writes: > "Daniel P. Berrange" writes: > >> Currently the QObjectInputVisitor assumes that all scalar >> values are directly represented as the final types declared >> by the thing being visited. ie it assumes an 'int' is using > > i.e. > >> QInt, and a 'bool' is using QBool, etc. This is good when >> QObjectInputVisitor is fed a QObject that came from a JSON >> document on the QMP monitor, as it will strictly validate >> correctness. >> >> To allow QObjectInputVisitor to be reused for visiting >> a QObject originating from QemuOpts, an alternative mode >> is needed where all the scalars types are represented as >> QString and converted on the fly to the final desired >> type. >> >> Reviewed-by: Kevin Wolf >> Reviewed-by: Marc-Andr=C3=A9 Lureau >> Signed-off-by: Daniel P. Berrange [...] >> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c >> index 5ff3db3..cf41df6 100644 >> --- a/qapi/qobject-input-visitor.c >> +++ b/qapi/qobject-input-visitor.c >> @@ -20,6 +20,7 @@ >> #include "qemu-common.h" >> #include "qapi/qmp/types.h" >> #include "qapi/qmp/qerror.h" >> +#include "qemu/cutils.h" >>=20=20 >> #define QIV_STACK_SIZE 1024 >>=20=20 >> @@ -263,6 +264,28 @@ static void qobject_input_type_int64(Visitor *v, co= nst char *name, int64_t *obj, >> *obj =3D qint_get_int(qint); >> } >>=20=20 >> + >> +static void qobject_input_type_int64_autocast(Visitor *v, const char *n= ame, >> + int64_t *obj, Error **err= p) >> +{ >> + QObjectInputVisitor *qiv =3D to_qiv(v); >> + QString *qstr =3D qobject_to_qstring(qobject_input_get_object(qiv, = name, >> + true)); > > Needs a rebase for commit 1382d4a. Same elsewhere. > >> + int64_t ret; >> + >> + if (!qstr || !qstr->string) { > > I don't think !qstr->string can happen. Same elsewhere. > >> + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nu= ll", >> + "string"); >> + return; >> + } > > So far, this is basically qobject_input_type_str() less the g_strdup(). > Worth factoring out? > > Now we're entering out parsing swamp: > >> + >> + if (qemu_strtoll(qstr->string, NULL, 0, &ret) < 0) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number"= ); "integer", actually. >> + return; >> + } > > To serve as replacement for the options visitor, this needs to parse > exactly like opts_type_int64(). > > It should also match the JSON parser as far as possible, to minimize > difference between the two QObject input visitor variants, and the > QemuOpts parser, for command line consistency. > > opts_type_int64() uses strtoll() directly. It carefully checks for no > conversion (both ways, EINVAL and endptr =3D=3D str), range, and string n= ot > fully consumed. > > Your code uses qemu_strtoll(). Bug#1: qemu_strtoll() assumes long long > is exactly 64 bits. If it's wider, we fail to diagnose overflow. If > it's narrower, we can't parse all values. Bug#2: your code fails to > check the string is fully consumed. > > The JSON parser also uses strtoll() directly, in parse_literal(). When > we get there, we know that the string consists only of decimal digits, > possibly prefixed by a minus sign. Therefore, strtoll() can only fail > with ERANGE, and parse_literal() handles that correctly. > > QemuOpts doesn't do signed integers. > >> + *obj =3D ret; >> +} >> + >> static void qobject_input_type_uint64(Visitor *v, const char *name, >> uint64_t *obj, Error **errp) >> { >> @@ -279,6 +302,27 @@ static void qobject_input_type_uint64(Visitor *v, c= onst char *name, >> *obj =3D qint_get_int(qint); >> } >>=20=20 >> +static void qobject_input_type_uint64_autocast(Visitor *v, const char *= name, >> + uint64_t *obj, Error **e= rrp) >> +{ >> + QObjectInputVisitor *qiv =3D to_qiv(v); >> + QString *qstr =3D qobject_to_qstring(qobject_input_get_object(qiv, = name, >> + true)); >> + unsigned long long ret; >> + >> + if (!qstr || !qstr->string) { >> + error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nu= ll", >> + "string"); >> + return; >> + } >> + >> + if (parse_uint_full(qstr->string, &ret, 0) < 0) { >> + error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name, "a number"= ); "integer". >> + return; >> + } >> + *obj =3D ret; > > Differently wrong :) > > To serve as replacement for the options visitor, this needs to parse > exactly like opts_type_uint64(). > > Again, this should also match the JSON parser and the QemuOpts parser as > far as possible. > > opts_type_uint64() uses parse_uint(). It carefully checks for no > conversion (EINVAL; parse_uint() normalizes), range, and string not > fully consumed. > > You use parse_uint_full(). You therefore don't have bug#2 here. You do > have bug#1: you assume unsigned long long is exactly 64 bits. If it's > wider, we fail to diagnose overflow. If it's narrower, we can't parse > all values. > > There's a discrepancy with the JSON parser, but it's not your fault: the > JSON parser represents JSON numbers that fit into int64_t to QInt, and > any others as QFloat. In particular, the integers between INT64_MAX+1 > and UINT64_MAX are represented as QFloat. qmp_input_type_uint64() > accepts only QInt. You can declare things as uint64 in the schema, but > you're still limited to 0..UINT64_MAX in QMP. > > QemuOpts uses strtoull() directly, in parse_option_number(). Bug (not > yours): it happily ignores errors other than "string not fully > consumed". > >> +} >> + >> static void qobject_input_type_bool(Visitor *v, const char *name, bool = *obj, >> Error **errp) >> { [...]