From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmj5z-0007Wt-4j for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:06:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmj5u-0008UQ-32 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:05:59 -0400 Received: from mx3-phx2.redhat.com ([209.132.183.24]:55443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmj5t-0008TX-Jn for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:05:54 -0400 Date: Wed, 21 Sep 2016 11:05:09 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <809451579.460846.1474470309806.JavaMail.zimbra@redhat.com> In-Reply-To: <87bmzhz5jx.fsf@dusky.pond.sub.org> References: <20160921103629.6410-1-marcandre.lureau@redhat.com> <20160921103629.6410-2-marcandre.lureau@redhat.com> <87bmzhz5jx.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 1/3] 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 > > --- > > qapi/qmp-input-visitor.c | 109 > > +++++++++++++++++++++++++++++++---------------- > > 1 file changed, 73 insertions(+), 36 deletions(-) > > > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > > index 64dd392..ea9972d 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; > > @@ -64,30 +64,34 @@ static QObject *qmp_input_get_object(QmpInputVisito= r > > *qiv, > > =20 > > if (QSLIST_EMPTY(&qiv->stack)) { > > /* Starting at root, name is ignored. */ > > - return qiv->root; > > - } >=20 > First case: not in a container. >=20 > qiv->root cannot be null. >=20 > The old code is relatively clear: it returns this non-null value. > Callers rely on it being non-null. I didn't realize that, ok >=20 > The new code muddies the waters: it handles the impossible null value by > setting an error with a misleading message, then returns null. >=20 > Please go back to the old code and simply return qiv->root. You may > assert it's non-null. ok > Two more cases: >=20 > * In a QTYPE_QDICT container: >=20 > If ret =3D qdict_get(qobject_to_qdict(qobj, name) is null, parameter > name is missing, and we want to > error_setg(errp, QERR_MISSING_PARAMETER, name). No ternary, because > name can't be null. >=20 > * In a QTYPE_QLIST container: >=20 > ret =3D qlist_entry_obj(tos->entry) is the list member, a QObject. It > must not be null because null is not a valid QObject. If we want to > catch this, we should assert, not set an error with a misleading > message. >=20 > Note for the rest of the review: we return null excactly when we set an > error. >=20 ok > > =20 > > @@ -163,13 +167,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; >=20 > Mechanical; the next hunk is the same pattern. >=20 > > @@ -191,10 +198,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; > > } > > @@ -232,11 +242,12 @@ 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 > > - if (!qobj) { > > + if (obj) { > > *obj =3D NULL; > > - error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null")= ; > > + } > > + if (!qobj) { > > return; > > } > > *obj =3D g_malloc0(size); >=20 > Why are you deviating from the mechanical change here? Because there is already a QERR_MISSING_PARAMETER return. >=20 > Note that obj can't be null here, by function contract. If called via > visit_start_alternate() as it should be, the contract is enforced there. ok >=20 > > @@ -250,8 +261,12 @@ 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 =3D qobject_to_qint(qobj); > > =20 > > + if (!qobj) { > > + return; > > + } >=20 > I'd call qobject_to_qint() here, not least for consistency with > qmp_input_type_number(). Of course, your code works, and if you feel > strongly about it, we can do it your way here. ok >=20 > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); >=20 > Mechanical; the next few hunks are the same pattern. >=20 > > @@ -266,8 +281,12 @@ 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 =3D qobject_to_qint(qobj); > > =20 > > + if (!qobj) { > > + return; > > + } > > if (!qint) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "integer"); > > @@ -281,8 +300,12 @@ 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 =3D qobject_to_qbool(qobj); > > =20 > > + if (!qobj) { > > + return; > > + } > > if (!qbool) { > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > "boolean"); > > @@ -296,8 +319,12 @@ static void qmp_input_type_str(Visitor *v, const c= har > > *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 =3D qobject_to_qstring(qobj); > > =20 > > + if (!qobj) { > > + return; > > + } > > if (!qstr) { > > *obj =3D NULL; > > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : > > "null", > > @@ -312,10 +339,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)); > > @@ -336,7 +366,11 @@ 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); > > + > > + if (!qobj) { > > + return; > > + } > > =20 > > qobject_incref(qobj); > > *obj =3D qobj; >=20 > Aha, we got a different bug fix! The old code fails to fail when the > parameter doesn't exist. Instead, it sets *obj =3D NULL, which seems ver= y > likely to crash QEMU. Let me try... yup: >=20 > { "execute": "object-add", > "arguments": { "qom-type": "memory-backend-file", "id": "foo" } } >=20 > Kills QEMU with "qemu/qom/object_interfaces.c:115: user_creatable_add_typ= e: > Assertion `qdict' failed." >=20 > Either fix this in a separate patch before this one, or cover it in this > one's commit message. Your choice. ok, I'll make a seperate patch >=20 > A separate patch might be usable for qemu-stable. >=20 > > @@ -345,8 +379,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. let's include it in the same patch >=20 > > @@ -356,7 +393,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 > Thanks for following my suggestion to move the "Parameter FOO is > missing" error into qmp_input_get_object()! You fixed two crash bugs > that way :) >=20