From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmaop-0004wy-Oh for qemu-devel@nongnu.org; Wed, 21 Sep 2016 02:15:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmaok-0002p5-J1 for qemu-devel@nongnu.org; Wed, 21 Sep 2016 02:15:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41180) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmaok-0002oh-5r for qemu-devel@nongnu.org; Wed, 21 Sep 2016 02:15:38 -0400 From: Markus Armbruster References: <20160912091913.15831-1-marcandre.lureau@redhat.com> <20160912091913.15831-16-marcandre.lureau@redhat.com> <20160920144844.nzbpfrauczlml3a5@perseus.local> <1441973931.277653.1474408728887.JavaMail.zimbra@redhat.com> <81179735.279206.1474408839927.JavaMail.zimbra@redhat.com> Date: Wed, 21 Sep 2016 08:15:34 +0200 In-Reply-To: <81179735.279206.1474408839927.JavaMail.zimbra@redhat.com> (=?utf-8?Q?=22Marc-Andr=C3=A9?= Lureau"'s message of "Tue, 20 Sep 2016 18:00:39 -0400 (EDT)") Message-ID: <8737ktaid5.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 v6 15/18] monitor: use qmp_dispatch() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Alberto Garcia , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau , qemu-devel@nongnu.org Marc-Andr=C3=A9 Lureau writes: > ----- Original Message ----- >> Hi Alberto >>=20 >> ----- Original Message ----- >> > On Mon, Sep 12, 2016 at 01:19:10PM +0400, Marc-Andr=C3=A9 Lureau wrote: >> > > Replace the old manual dispatch and validation code by the generic o= ne >> > > provided by qapi common code. >> > >=20 >> > > Note that it is now possible to call the following commands that use= d to >> > > be disabled by compile-time conditionals: >> > > - dump-skeys >> > > - query-spice >> > > - rtc-reset-reinjection >> > > - query-gic-capabilities >> > >=20 >> > > Their fallback functions return an appropriate "feature disabled" er= ror. >> > >=20 >> > > Signed-off-by: Marc-Andr=C3=A9 Lureau >> >=20 >> > This patch breaks iotest 085 because the "missing parameter" error is >> > now different: >> >=20 >> > -{"error": {"class": "GenericError", "desc": "Parameter 'snapshot-file= ' is >> > missing"}} >> > +{"error": {"class": "GenericError", "desc": "Invalid parameter type f= or >> > 'snapshot-file', expected: string"}} >> >=20 >> > I was thinking to update the expected output of the iotest, but I >> > guess it's better to return a more meaningful error message? >>=20 >> The change is relatively easy (see attached patch), but there are other = tests >> that expected the current error, see commit fe509ee237307843. I guess we >> should decided with one, and I think missing parameter is more appropria= te. Agree. > diff attached > > diff --git a/qapi/qmp-input-visitor.c b/qapi/qmp-input-visitor.c > index 64dd392..22b132b 100644 > --- a/qapi/qmp-input-visitor.c > +++ b/qapi/qmp-input-visitor.c > @@ -169,7 +169,11 @@ static void qmp_input_start_struct(Visitor *v, const= char *name, void **obj, > if (obj) { > *obj =3D NULL; > } > - if (!qobj || qobject_type(qobj) !=3D QTYPE_QDICT) { > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > + if (qobject_type(qobj) !=3D QTYPE_QDICT) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > "QDict"); > return; > @@ -194,7 +198,11 @@ static void qmp_input_start_list(Visitor *v, const c= har *name, > QObject *qobj =3D qmp_input_get_object(qiv, name, true); > const QListEntry *entry; >=20=20 > - if (!qobj || qobject_type(qobj) !=3D QTYPE_QLIST) { > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > + if (qobject_type(qobj) !=3D QTYPE_QLIST) { > if (list) { > *list =3D NULL; > } > @@ -234,8 +242,10 @@ static void qmp_input_start_alternate(Visitor *v, co= nst char *name, > QmpInputVisitor *qiv =3D to_qiv(v); > QObject *qobj =3D qmp_input_get_object(qiv, name, false); >=20=20 > - if (!qobj) { > + if (obj) { > *obj =3D NULL; > + } > + if (!qobj) { > error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > return; > } > @@ -250,8 +260,13 @@ static void qmp_input_type_int64(Visitor *v, const c= har *name, int64_t *obj, > Error **errp) > { > QmpInputVisitor *qiv =3D to_qiv(v); > - QInt *qint =3D qobject_to_qint(qmp_input_get_object(qiv, name, true)= ); > + QObject *qobj =3D qmp_input_get_object(qiv, name, true); > + QInt *qint =3D qobject_to_qint(qobj); >=20=20 > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > if (!qint) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > "integer"); > @@ -266,8 +281,13 @@ static void qmp_input_type_uint64(Visitor *v, const = 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, true)= ); > + QObject *qobj =3D qmp_input_get_object(qiv, name, true); > + QInt *qint =3D qobject_to_qint(qobj); >=20=20 > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > if (!qint) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > "integer"); > @@ -281,8 +301,13 @@ static void qmp_input_type_bool(Visitor *v, const ch= ar *name, bool *obj, > Error **errp) > { > QmpInputVisitor *qiv =3D to_qiv(v); > - QBool *qbool =3D qobject_to_qbool(qmp_input_get_object(qiv, name, tr= ue)); > + QObject *qobj =3D qmp_input_get_object(qiv, name, true); > + QBool *qbool =3D qobject_to_qbool(qobj); >=20=20 > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > if (!qbool) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > "boolean"); > @@ -296,8 +321,16 @@ static void qmp_input_type_str(Visitor *v, const cha= r *name, char **obj, > Error **errp) > { > QmpInputVisitor *qiv =3D to_qiv(v); > - QString *qstr =3D qobject_to_qstring(qmp_input_get_object(qiv, name,= true)); > + QObject *qobj =3D qmp_input_get_object(qiv, name, true); > + QString *qstr =3D qobject_to_qstring(qobj); >=20=20 > + if (obj) { > + *obj =3D NULL; > + } > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > if (!qstr) { > *obj =3D NULL; > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > @@ -316,6 +349,10 @@ static void qmp_input_type_number(Visitor *v, const = char *name, double *obj, > QInt *qint; > QFloat *qfloat; >=20=20 > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > qint =3D qobject_to_qint(qobj); > if (qint) { > *obj =3D qint_get_int(qobject_to_qint(qobj)); > @@ -338,6 +375,14 @@ static void qmp_input_type_any(Visitor *v, const cha= r *name, QObject **obj, > QmpInputVisitor *qiv =3D to_qiv(v); > QObject *qobj =3D qmp_input_get_object(qiv, name, true); >=20=20 > + if (obj) { > + *obj =3D NULL; > + } > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > + > qobject_incref(qobj); > *obj =3D qobj; > } > @@ -347,6 +392,10 @@ static void qmp_input_type_null(Visitor *v, const ch= ar *name, Error **errp) > QmpInputVisitor *qiv =3D to_qiv(v); > QObject *qobj =3D qmp_input_get_object(qiv, name, true); >=20=20 > + if (!qobj) { > + error_setg(errp, QERR_MISSING_PARAMETER, name ? name : "null"); > + return; > + } > if (qobject_type(qobj) !=3D QTYPE_QNULL) { > error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "nul= l", > "null"); Changing qmp_input_get_object() to take an Error * parameter and set the "Parameter FOO is missing" error could require less code. Suggest to give it a try and see how you like it. Need a proper patch to merge this regression fix.