From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bn3ad-0008V7-C3 for qemu-devel@nongnu.org; Thu, 22 Sep 2016 08:59:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bn3aX-0000aF-CQ for qemu-devel@nongnu.org; Thu, 22 Sep 2016 08:58:58 -0400 Received: from mx6-phx2.redhat.com ([209.132.183.39]:48590) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bn3aX-0000Zn-2o for qemu-devel@nongnu.org; Thu, 22 Sep 2016 08:58:53 -0400 Date: Thu, 22 Sep 2016 08:58:11 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1605746177.687490.1474549091975.JavaMail.zimbra@redhat.com> In-Reply-To: <87lgyk9k6h.fsf@dusky.pond.sub.org> References: <20160921201018.20957-1-marcandre.lureau@redhat.com> <20160921201018.20957-4-marcandre.lureau@redhat.com> <87lgyk9k6h.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 v2 3/4] qapi: return a 'missing parameter' error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org, berto@igalia.com Hi ----- Original Message ----- > Marc-Andr=C3=A9 Lureau writes: >=20 > > The 'old' dispatch code returned a QERR_MISSING_PARAMETER for missing > > parameters, but the qapi qmp_dispatch() code uses > > QERR_INVALID_PARAMETER_TYPE. > > > > Improve qapi code to return QERR_INVALID_PARAMETER_TYPE where > > appropriate. > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > Reviewed-by: Alberto Garcia > > --- > > qapi/qmp-input-visitor.c | 73 > > +++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 56 insertions(+), 17 deletions(-) > > > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > > index c1019d6..6f85664 100644 > > --- a/qapi/qmp-input-visitor.c > > +++ b/qapi/qmp-input-visitor.c > > @@ -56,7 +56,7 @@ static QmpInputVisitor *to_qiv(Visitor *v) > > =20 > > static QObject *qmp_input_get_object(QmpInputVisitor *qiv, > > const char *name, > > - bool consume) > > + bool consume, Error **errp) > > { > > StackObject *tos; > > QObject *qobj; > > @@ -80,6 +80,9 @@ static QObject *qmp_input_get_object(QmpInputVisitor > > *qiv, > > bool removed =3D g_hash_table_remove(tos->h, name); > > assert(removed); > > } > > + if (!ret) { > > + error_setg(errp, QERR_MISSING_PARAMETER, name); > > + } > > } else { > > assert(qobject_type(qobj) =3D=3D QTYPE_QLIST); > > assert(!name); > > @@ -165,13 +168,16 @@ static void qmp_input_start_struct(Visitor *v, co= nst > > char *name, void **obj, > > size_t size, Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, true); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > Error *err =3D NULL; > > =20 > > if (obj) { > > *obj =3D NULL; > > } > > - if (!qobj || qobject_type(qobj) !=3D QTYPE_QDICT) { > > + if (!qobj) { > > + return; > > + } > > + if (qobject_type(qobj) !=3D QTYPE_QDICT) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "QDict"); > > return; > > @@ -193,10 +199,13 @@ static void qmp_input_start_list(Visitor *v, cons= t > > char *name, > > GenericList **list, size_t size, Erro= r > > **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, true); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > const QListEntry *entry; > > =20 > > - if (!qobj || qobject_type(qobj) !=3D QTYPE_QLIST) { > > + if (!qobj) { > > + return; > > + } > > + if (qobject_type(qobj) !=3D QTYPE_QLIST) { > > if (list) { > > *list =3D NULL; > > } > > @@ -234,11 +243,10 @@ static void qmp_input_start_alternate(Visitor *v, > > const char *name, > > bool promote_int, Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, false); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, false, errp); > > =20 > > + *obj =3D NULL; > > if (!qobj) { > > - *obj =3D NULL; > > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null")= ; > > return; > > } > > *obj =3D g_malloc0(size); >=20 > The patch does more than one thing: in addition to fixing the 'missing > parameter' regression, it also messes with *obj =3D NULL in places. Thes= e > changes may well make sense, but they should be a separate patch, to > ease review. Looking more at it, I think we can leave it only in the error case. >=20 > > @@ -252,8 +260,13 @@ static void qmp_input_type_int64(Visitor *v, const > > char *name, int64_t *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QInt *qint =3D qobject_to_qint(qmp_input_get_object(qiv, name, tru= e)); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > + QInt *qint; > > =20 > > + if (!qobj) { > > + return; > > + } > > + qint =3D qobject_to_qint(qobj); > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); > > @@ -268,8 +281,13 @@ static void qmp_input_type_uint64(Visitor *v, cons= t > > char *name, uint64_t *obj, > > { > > /* FIXME: qobject_to_qint mishandles values over INT64_MAX */ > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QInt *qint =3D qobject_to_qint(qmp_input_get_object(qiv, name, tru= e)); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > + QInt *qint; > > =20 > > + if (!qobj) { > > + return; > > + } > > + qint =3D qobject_to_qint(qobj); > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); > > @@ -283,8 +301,13 @@ static void qmp_input_type_bool(Visitor *v, const = char > > *name, bool *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QBool *qbool =3D qobject_to_qbool(qmp_input_get_object(qiv, name, > > true)); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > + QBool *qbool; > > =20 > > + if (!qobj) { > > + return; > > + } > > + qbool =3D qobject_to_qbool(qobj); > > if (!qbool) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "boolean"); > > @@ -298,10 +321,15 @@ static void qmp_input_type_str(Visitor *v, const = char > > *name, char **obj, > > Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QString *qstr =3D qobject_to_qstring(qmp_input_get_object(qiv, nam= e, > > true)); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > + QString *qstr; > > =20 > > + *obj =3D NULL; > > + if (!qobj) { > > + return; > > + } > > + qstr =3D qobject_to_qstring(qobj); > > if (!qstr) { > > - *obj =3D NULL; > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "string"); > > return; > > @@ -314,10 +342,13 @@ static void qmp_input_type_number(Visitor *v, con= st > > char *name, double *obj, > > Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, true); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > QInt *qint; > > QFloat *qfloat; > > =20 > > + if (!qobj) { > > + return; > > + } > > qint =3D qobject_to_qint(qobj); > > if (qint) { > > *obj =3D qint_get_int(qobject_to_qint(qobj)); > > @@ -338,7 +369,12 @@ static void qmp_input_type_any(Visitor *v, const c= har > > *name, QObject **obj, > > Error **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, true); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > + > > + *obj =3D NULL; > > + if (!qobj) { > > + return; > > + } > > =20 > > qobject_incref(qobj); > > *obj =3D qobj; >=20 > The patch does a third thing: it fixes a crash bug. The old code fails > to fail when the parameter doesn't exist. Instead, it sets *obj =3D NULL= , > violating its contract. Reproducer: >=20 > { "execute": "qom-set", > "arguments": { "path": "/machine", "property": "rtc-time" } } >=20 > Separate patch, please, cc: qemu-stable. >=20 We can make it a seperate patch, but we still need to return an error, that= 's what this patch does. A seperate patch will look like: @@ -338,6 +338,12 @@ static void qmp_input_type_any(Visitor *v, const char = *name, QObject **obj, QmpInputVisitor *qiv =3D to_qiv(v); QObject *qobj =3D qmp_input_get_object(qiv, name, true); =20 + if (!qobj) { + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); + *obj =3D NULL; + return; + } I think we may as well just take this patch, what do you think? > > @@ -347,8 +383,11 @@ static void qmp_input_type_any(Visitor *v, const c= har > > *name, QObject **obj, > > static void qmp_input_type_null(Visitor *v, const char *name, Error > > **errp) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, true); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, true, errp); > > =20 > > + if (!qobj) { > > + return; > > + } > > if (qobject_type(qobj) !=3D QTYPE_QNULL) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "null"); >=20 > Same bug, I think, but I don't have a reproducer handy. >=20 > > @@ -358,7 +397,7 @@ static void qmp_input_type_null(Visitor *v, const c= har > > *name, Error **errp) > > static void qmp_input_optional(Visitor *v, const char *name, bool > > *present) > > { > > QmpInputVisitor *qiv =3D to_qiv(v); > > - QObject *qobj =3D qmp_input_get_object(qiv, name, false); > > + QObject *qobj =3D qmp_input_get_object(qiv, name, false, NULL); > > =20 > > if (!qobj) { > > *present =3D false; >=20