From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bX61I-0000ed-M4 for qemu-devel@nongnu.org; Tue, 09 Aug 2016 08:20:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bX61C-0001z8-60 for qemu-devel@nongnu.org; Tue, 09 Aug 2016 08:20:31 -0400 Received: from mx6-phx2.redhat.com ([209.132.183.39]:53026) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bX61B-0001yx-S5 for qemu-devel@nongnu.org; Tue, 09 Aug 2016 08:20:26 -0400 Date: Tue, 9 Aug 2016 08:20:22 -0400 (EDT) From: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Message-ID: <1694406019.558113.1470745222825.JavaMail.zimbra@redhat.com> In-Reply-To: <87invagn07.fsf@dusky.pond.sub.org> References: <20160808141439.16908-1-marcandre.lureau@redhat.com> <20160808141439.16908-11-marcandre.lureau@redhat.com> <87invagn07.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 v3 10/15] qapi: check invalid arguments on no-args commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre lureau , qemu-devel@nongnu.org Hi ----- Original Message ----- > marcandre.lureau@redhat.com writes: >=20 > > From: Marc-Andr=C3=A9 Lureau > > > > The generated marshal functions do not visit arguments from commands > > that take no arguments. Thus they fail to catch invalid > > members. Visit the arguments, if provided, to throw an error in case of > > invalid members. >=20 > Begs the question why this isn't a bug fix :) >=20 > For QMP, it isn't, because check_client_args_type() does the same work. > It's called by handle_qmp_command() via qmp_check_client_args(). You > remove it in PATCH 12, and that's why you tighten the marshaling > functions' invalid arguments checking now. >=20 > What about QGA? >=20 > Back to QMP. check_client_args_type() checks against the args_type > defined in qmp-commands.hx. Your generated code checks against the > schema. Good, because the args_type duplicate schema information, and > your series gets rid of it. However, there's a funny special case we > need to consider: args_type character 'O' accepts arbitary arguments. > check_client_args_type() skips the check for unknown arguments then. > This requires commands using 'O' to have 'gen': false in the schema. > This is the case. >=20 > The commit message should explain these things. ok >=20 > > static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp) > > { > > + Visitor *v =3D NULL; > > Error *err =3D NULL; > > - > > - (void)args; > > + if (args) { > > + v =3D qmp_input_visitor_new(QOBJECT(args), true); > > + visit_start_struct(v, NULL, NULL, 0, &err); > > + if (err) { > > + goto out; > > + } > > + > > + if (!err) { > > + visit_check_struct(v, &err); > > + } > > + visit_end_struct(v, NULL); > > + if (err) { > > + goto out; > > + } > > + } > > > > qmp_stop(&err); > > + > > +out: > > error_propagate(errp, err); > > + visit_free(v); > > + if (args) { > > + v =3D qapi_dealloc_visitor_new(); > > + visit_start_struct(v, NULL, NULL, 0, NULL); > > + > > + visit_end_struct(v, NULL); > > + visit_free(v); > > + } > > } >=20 > The new code closely resembles code for a command with arguments. > That's a good sign. Differences: >=20 > * The visit of the argument and its cleanup struct don't visit any > members (because there are none). >=20 > * The visit of the argument struct and its cleanup are conditional. >=20 > I think that's also worth explaining in the commit message. >=20 > However, this is not the only change to generated code: marshalers for > commands with arguments also change, like this: >=20 > static void qmp_marshal_netdev_del(QDict *args, QObject **ret, Error > **errp) > { > + Visitor *v =3D NULL; > Error *err =3D NULL; > - Visitor *v; > q_obj_netdev_del_arg arg =3D {0}; >=20 > v =3D qmp_input_visitor_new(QOBJECT(args), true); >=20 > The initialization is dead for commands with arguments. For commands > without arguments, it suppresses a bogus "may be used uninitialized" > warning. Oh well. >=20 > > > > Signed-off-by: Marc-Andr=C3=A9 Lureau > > --- > > tests/test-qmp-commands.c | 15 ++++++++++++++ > > scripts/qapi-commands.py | 51 > > ++++++++++++++++++++++++++++++----------------- > > 2 files changed, 48 insertions(+), 18 deletions(-) > > > > diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c > > index 261fd9e..81cbe54 100644 > > --- a/tests/test-qmp-commands.c > > +++ b/tests/test-qmp-commands.c >=20 > You remembered to add a test. Appreciated! >=20 > > @@ -106,6 +106,7 @@ static void test_dispatch_cmd(void) > > static void test_dispatch_cmd_failure(void) > > { > > QDict *req =3D qdict_new(); > > + QDict *args =3D qdict_new(); > > QObject *resp; > > =20 > > qdict_put_obj(req, "execute", > > QOBJECT(qstring_from_str("user_def_cmd2"))); > > @@ -114,6 +115,20 @@ static void test_dispatch_cmd_failure(void) > > assert(resp !=3D NULL); > > assert(qdict_haskey(qobject_to_qdict(resp), "error")); > > =20 > > + qobject_decref(resp); > > + QDECREF(req); > > + > > + /* check that with extra arguments it throws an error */ > > + req =3D qdict_new(); > > + qdict_put(args, "a", qint_from_int(66)); > > + qdict_put(req, "arguments", args); > > + > > + qdict_put_obj(req, "execute", > > QOBJECT(qstring_from_str("user_def_cmd"))); >=20 > Matches how the other request objects are built. qobject_from_json() > could be simpler, though. Your choice. >=20 > > + > > + resp =3D qmp_dispatch(QOBJECT(req)); > > + assert(resp !=3D NULL); > > + assert(qdict_haskey(qobject_to_qdict(resp), "error")); > > + > > qobject_decref(resp); > > QDECREF(req); > > } > > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > > index 11aa54b..735e463 100644 > > --- a/scripts/qapi-commands.py > > +++ b/scripts/qapi-commands.py > > @@ -97,11 +97,27 @@ def gen_marshal_decl(name, export): > > proto=3Dgen_marshal_proto(name, export)) > > =20 > > =20 > > +def if_args(arg_type, block): > > + ret =3D '' > > + if not arg_type or arg_type.is_empty(): > > + ret +=3D mcgen(''' > > + if (args) { > > +''') > > + push_indent() > > + ret +=3D block() > > + if not arg_type or arg_type.is_empty(): > > + pop_indent() > > + ret +=3D mcgen(''' > > + } > > +''') > > + return ret > > + > > def gen_marshal(name, arg_type, boxed, ret_type, export): > > ret =3D mcgen(''' > > =20 > > %(proto)s > > { > > + Visitor *v =3D NULL; > > Error *err =3D NULL; > > ''', > > proto=3Dgen_marshal_proto(name, export)) > > @@ -112,17 +128,23 @@ def gen_marshal(name, arg_type, boxed, ret_type, > > export): > > ''', > > c_type=3Dret_type.c_type()) > > =20 > > + visit_members =3D '' > > if arg_type and not arg_type.is_empty(): > > + visit_members =3D mcgen('visit_type_%(c_name)s_members(v, &arg= , > > &err);', > > + c_name=3Darg_type.c_name()) > > ret +=3D mcgen(''' > > - Visitor *v; > > %(c_name)s arg =3D {0}; > > =20 > > +''', > > + c_name=3Darg_type.c_name()) > > + > > + ret +=3D if_args(arg_type, lambda: mcgen(''' > > v =3D qmp_input_visitor_new(QOBJECT(args), true); > > visit_start_struct(v, NULL, NULL, 0, &err); > > if (err) { > > goto out; > > } > > - visit_type_%(c_name)s_members(v, &arg, &err); > > + %(visit_members)s >=20 > Uh, visit_members is output of mcgen(), so this feeds output of mcgen() > to mcgen() again. Not good, because any interpolations done by the > second mcgen() are almost certainly not wanted. Likewise indentation. >=20 > The easiest fix is to say >=20 > visit_members =3D ('visit_type_%(c_name)s_members(v, &arg, &err);= ' > % arg_type.c_name()) >=20 ok > > if (!err) { > > visit_check_struct(v, &err); > > } > > @@ -131,35 +153,28 @@ def gen_marshal(name, arg_type, boxed, ret_type, > > export): > > goto out; > > } > > ''', > > - c_name=3Darg_type.c_name()) > > - > > - else: > > - ret +=3D mcgen(''' > > - > > - (void)args; > > -''') > > + visit_members=3Dvisit_members)) > > =20 > > ret +=3D gen_call(name, arg_type, boxed, ret_type) > > =20 > > # 'goto out' produced above for arg_type, and by gen_call() for > > ret_type >=20 > I think this comment becomes wrong, and should be dropped. >=20 > > - if (arg_type and not arg_type.is_empty()) or ret_type: > > - ret +=3D mcgen(''' > > + if arg_type and not arg_type.is_empty(): > > + visit_members =3D mcgen('visit_type_%(c_name)s_members(v, &arg= , > > NULL);', > > + c_name=3Darg_type.c_name()) > > + ret +=3D mcgen(''' > > =20 > > out: > > -''') > > - ret +=3D mcgen(''' > > error_propagate(errp, err); > > -''') > > - if arg_type and not arg_type.is_empty(): > > - ret +=3D mcgen(''' > > visit_free(v); > > +''') > > + ret +=3D if_args(arg_type, lambda: mcgen(''' > > v =3D qapi_dealloc_visitor_new(); > > visit_start_struct(v, NULL, NULL, 0, NULL); > > - visit_type_%(c_name)s_members(v, &arg, NULL); > > + %(visit_members)s > > visit_end_struct(v, NULL); > > visit_free(v); > > ''', > > - c_name=3Darg_type.c_name()) > > + visit_members=3Dvisit_members)) > > =20 > > ret +=3D mcgen(''' > > } >=20 > Encapsulating the conditional in if_args() and passing it lambda > arguments is kind of cute, but is it worthwhile? To find out, I rewrote > this part the stupidest way I could, diff appended. Turns out it's the > same amount of code, only stupider code. >=20 > It also avoids the dead initialization of v, because it's easy to do. >=20 > I'm fine with clever when it saves repetition, but when it doesn't, I'll > have stupid, please. Well the differences is that in case there are no command args, then you ca= n pass NULL to the function, while if there are expected args, you must pas= s args (or it will fail to report missing args actually). The if_args() was= n't just for adding "if (args)" but also to check if there are expected arg= s (see also the diff before/after)=20 >=20 > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py > index 11aa54b..2e6cdb9 100644 > --- a/scripts/qapi-commands.py > +++ b/scripts/qapi-commands.py > @@ -117,12 +117,26 @@ def gen_marshal(name, arg_type, boxed, ret_type, > export): > Visitor *v; > %(c_name)s arg =3D {0}; > =20 > +''', > + c_name=3Darg_type.c_name()) > + visit_members =3D ('visit_type_%s_members(v, &arg, &err);' > + % arg_type.c_name()) > + else: > + ret +=3D mcgen(''' > + Visitor *v =3D NULL; > + > + if (args) { > +''') > + push_indent(); > + visit_members =3D '' > + > + ret +=3D mcgen(''' > v =3D qmp_input_visitor_new(QOBJECT(args), true); > visit_start_struct(v, NULL, NULL, 0, &err); > if (err) { > goto out; > } > - visit_type_%(c_name)s_members(v, &arg, &err); > + %(visit_members)s > if (!err) { > visit_check_struct(v, &err); > } > @@ -131,35 +145,39 @@ def gen_marshal(name, arg_type, boxed, ret_type, > export): > goto out; > } > ''', > - c_name=3Darg_type.c_name()) > - > - else: > + visit_members=3Dvisit_members) > + if not arg_type or arg_type.is_empty(): > + pop_indent() > ret +=3D mcgen(''' > - > - (void)args; > + } > ''') > =20 > ret +=3D gen_call(name, arg_type, boxed, ret_type) > =20 > - # 'goto out' produced above for arg_type, and by gen_call() for ret_= type > - if (arg_type and not arg_type.is_empty()) or ret_type: > - ret +=3D mcgen(''' > + ret +=3D mcgen(''' > =20 > out: > -''') > - ret +=3D mcgen(''' > error_propagate(errp, err); > ''') > - if arg_type and not arg_type.is_empty(): > + if not arg_type or arg_type.is_empty(): > ret +=3D mcgen(''' > + if (args) { > +''') > + push_indent(); > + ret +=3D mcgen(''' > visit_free(v); > v =3D qapi_dealloc_visitor_new(); > visit_start_struct(v, NULL, NULL, 0, NULL); > - visit_type_%(c_name)s_members(v, &arg, NULL); > + %(visit_members)s > visit_end_struct(v, NULL); > visit_free(v); > -''', > - c_name=3Darg_type.c_name()) > + ''', > + visit_members=3Dvisit_members) > + if not arg_type or arg_type.is_empty(): > + pop_indent() > + ret +=3D mcgen(''' > + } > +''') > =20 > ret +=3D mcgen(''' > } >=20