[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-ar
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-args commands |
Date: |
Wed, 17 Aug 2016 15:49:27 +0000 |
Hi
On Wed, Aug 17, 2016 at 6:49 PM Markus Armbruster <address@hidden> wrote:
> address@hidden writes:
>
> > From: Marc-André Lureau <address@hidden>
> >
> > 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.
> >
> > Currently, qmp_check_client_args() checks for invalid arguments and
> > correctly catches this case. When switching to qmp_dispatch() we want to
> > keep that behaviour. The commands using 'O' may have arbitrary
> > arguments, and must have 'gen': false in the qapi schema to skip the
> > generated checks.
>
> Explains why this isn't a bug fix for QMP. What about QGA?
>
Sorry, I don't understand what you ask. I thought the above paragraph that
I added described the current QMP behaviour and why we want to keep it. And
yes, it'a fix for qga too (it's qapi)
>
> > Old/new diff:
> > static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp)
> > {
> > + Visitor *v = NULL;
> > Error *err = NULL;
> > -
>
> Please keep the blank line between declarations and statements.
>
>
> > - (void)args;
> > + if (args) {
>
> This code...
>
> > + v = 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;
> > + }
>
> ... is not indented in my build.
>
> > + }
> >
> > qmp_stop(&err);
> > +
> > +out:
> > error_propagate(errp, err);
> > + visit_free(v);
> > + if (args) {
> > + v = qapi_dealloc_visitor_new();
> > + visit_start_struct(v, NULL, NULL, 0, NULL);
> > +
> > + visit_end_struct(v, NULL);
> > + visit_free(v);
>
> Likewise.
>
> > + }
> > }
> >
> > The new code closely resembles code for a command with arguments.
> > Differences:
> > - the visit of the argument and its cleanup struct don't visit any
> > members (because there are none).
> > - the visit of the argument struct and its cleanup are conditional.
>
> Additional diff for all other qmp_marshal_FOO():
>
> @@ -46,9 +46,9 @@ static void qmp_marshal_output_AddfdInfo
>
> void qmp_marshal_add_fd(QDict *args, QObject **ret, Error **errp)
> {
> + Visitor *v = NULL;
> Error *err = NULL;
> AddfdInfo *retval;
> - Visitor *v;
> q_obj_add_fd_arg arg = {0};
>
> v = qmp_input_visitor_new(QOBJECT(args), true);
>
> Let's avoid this churn.
>
> >
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> > tests/test-qmp-commands.c | 15 ++++++++++++++
> > scripts/qapi-commands.py | 53
> ++++++++++++++++++++++++++++++-----------------
> > 2 files changed, 49 insertions(+), 19 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
> > @@ -106,6 +106,7 @@ static void test_dispatch_cmd(void)
> > static void test_dispatch_cmd_failure(void)
> > {
> > QDict *req = qdict_new();
> > + QDict *args = qdict_new();
> > QObject *resp;
> >
> > 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 != NULL);
> > assert(qdict_haskey(qobject_to_qdict(resp), "error"));
> >
> > + qobject_decref(resp);
> > + QDECREF(req);
> > +
> > + /* check that with extra arguments it throws an error */
> > + req = 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")));
> > +
> > + resp = qmp_dispatch(QOBJECT(req));
> > + assert(resp != 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 eac64ce..9c79b18 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -94,11 +94,28 @@ def gen_marshal_decl(name):
> > proto=gen_marshal_proto(name))
> >
> >
> > +def if_args(arg_type, block):
> > + ret = ''
> > + if not arg_type or arg_type.is_empty():
> > + ret += mcgen('''
> > + if (args) {
> > +''')
> > + push_indent()
> > + ret += block
> > + if not arg_type or arg_type.is_empty():
> > + pop_indent()
> > + ret += mcgen('''
> > + }
> > +''')
>
> Since @block has already been formatted, the push_indent() /
> pop_indent() has no effect. I guess that's why you did it with a lambda
> in v3.
>
> > + return ret
> > +
> > +
> > def gen_marshal(name, arg_type, boxed, ret_type):
> > ret = mcgen('''
> >
> > %(proto)s
> > {
> > + Visitor *v = NULL;
> > Error *err = NULL;
> > ''',
> > proto=gen_marshal_proto(name))
> > @@ -109,17 +126,23 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> > ''',
> > c_type=ret_type.c_type())
> >
> > + visit_members = ''
> > if arg_type and not arg_type.is_empty():
> > + visit_members = 'visit_type_%s_members(v, &arg, &err);' % \
> > + arg_type.c_name()
>
> PEP8 prefers
>
> visit_members = ('visit_type_%s_members(v, &arg, &err);'
> % arg_type.c_name())
>
> > ret += mcgen('''
> > - Visitor *v;
> > %(c_name)s arg = {0};
> >
> > +''',
> > + c_name=arg_type.c_name())
> > +
> > + ret += if_args(arg_type, mcgen('''
> > v = 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);
> > }
> > @@ -128,35 +151,27 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> > goto out;
> > }
> > ''',
> > - c_name=arg_type.c_name())
> > -
> > - else:
> > - ret += mcgen('''
> > -
> > - (void)args;
> > -''')
> > + visit_members=visit_members))
> >
> > ret += gen_call(name, arg_type, boxed, ret_type)
> >
> > - # '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 += mcgen('''
> > + if arg_type and not arg_type.is_empty():
> > + visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg,
> NULL);',
> > + c_name=arg_type.c_name())
>
> I'm afraid you missed this instance of "mcgen()'s output fed to
> mcgen()".
>
> > + ret += mcgen('''
> >
> > out:
> > -''')
> > - ret += mcgen('''
> > error_propagate(errp, err);
> > -''')
> > - if arg_type and not arg_type.is_empty():
> > - ret += mcgen('''
> > visit_free(v);
> > +''')
> > + ret += if_args(arg_type, mcgen('''
> > v = 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=arg_type.c_name())
> > + visit_members=visit_members))
> >
> > ret += mcgen('''
> > }
>
> I append a diff from this one to a stupider solution. It's slightly
> longer, but I find it a bit easier to understand.
>
ok, applied
>
>
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index 9c79b18..2f603b0 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -94,28 +94,13 @@ def gen_marshal_decl(name):
> proto=gen_marshal_proto(name))
>
>
> -def if_args(arg_type, block):
> - ret = ''
> - if not arg_type or arg_type.is_empty():
> - ret += mcgen('''
> - if (args) {
> -''')
> - push_indent()
> - ret += block
> - if not arg_type or arg_type.is_empty():
> - pop_indent()
> - ret += mcgen('''
> - }
> -''')
> - return ret
> -
> -
> def gen_marshal(name, arg_type, boxed, ret_type):
> + have_args = arg_type and not arg_type.is_empty()
> +
> ret = mcgen('''
>
> %(proto)s
> {
> - Visitor *v = NULL;
> Error *err = NULL;
> ''',
> proto=gen_marshal_proto(name))
> @@ -126,17 +111,25 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> ''',
> c_type=ret_type.c_type())
>
> - visit_members = ''
> - if arg_type and not arg_type.is_empty():
> - visit_members = 'visit_type_%s_members(v, &arg, &err);' % \
> - arg_type.c_name()
> + if have_args:
> + visit_members = ('visit_type_%s_members(v, &arg, &err);'
> + % arg_type.c_name())
> ret += mcgen('''
> + Visitor *v;
> %(c_name)s arg = {0};
>
> ''',
> c_name=arg_type.c_name())
> + else:
> + visit_members = ''
> + ret += mcgen('''
> + Visitor *v = NULL;
>
> - ret += if_args(arg_type, mcgen('''
> + if (args) {
> +''')
> + push_indent()
> +
> + ret += mcgen('''
> v = qmp_input_visitor_new(QOBJECT(args), true);
> visit_start_struct(v, NULL, NULL, 0, &err);
> if (err) {
> @@ -151,27 +144,47 @@ def gen_marshal(name, arg_type, boxed, ret_type):
> goto out;
> }
> ''',
> - visit_members=visit_members))
> + visit_members=visit_members)
> +
> + if not have_args:
> + pop_indent()
> + ret += mcgen('''
> + }
> +''')
>
> ret += gen_call(name, arg_type, boxed, ret_type)
>
> - if arg_type and not arg_type.is_empty():
> - visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg,
> NULL);',
> - c_name=arg_type.c_name())
> ret += mcgen('''
>
> out:
> error_propagate(errp, err);
> visit_free(v);
> ''')
> - ret += if_args(arg_type, mcgen('''
> +
> + if have_args:
> + visit_members = ('visit_type_%s_members(v, &arg, NULL);'
> + % arg_type.c_name())
> + else:
> + visit_members = ''
> + ret += mcgen('''
> + if (args) {
> +''')
> + push_indent()
> +
> + ret += mcgen('''
> v = qapi_dealloc_visitor_new();
> visit_start_struct(v, NULL, NULL, 0, NULL);
> %(visit_members)s
> visit_end_struct(v, NULL);
> visit_free(v);
> ''',
> - visit_members=visit_members))
> + visit_members=visit_members)
> +
> + if not have_args:
> + pop_indent()
> + ret += mcgen('''
> + }
> +''')
>
> ret += mcgen('''
> }
>
> --
Marc-André Lureau
- Re: [Qemu-devel] [PATCH v4 06/17] monitor: unregister conditional commands, (continued)
- [Qemu-devel] [PATCH v4 07/17] qapi: export the marshallers, marcandre . lureau, 2016/08/10
- [Qemu-devel] [PATCH v4 08/17] monitor: use qmp_find_command() (using generated qapi code), marcandre . lureau, 2016/08/10
- [Qemu-devel] [PATCH v4 09/17] monitor: implement 'qmp_query_commands' without qmp_cmds, marcandre . lureau, 2016/08/10
- [Qemu-devel] [PATCH v4 11/17] qapi: remove the "middle" mode, marcandre . lureau, 2016/08/10
- [Qemu-devel] [PATCH v4 10/17] monitor: remove mhandler.cmd_new, marcandre . lureau, 2016/08/10
- [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-args commands, marcandre . lureau, 2016/08/10
[Qemu-devel] [PATCH v4 13/17] qmp: update qmp_query_spice fallback, marcandre . lureau, 2016/08/10
[Qemu-devel] [PATCH v4 14/17] monitor: use qmp_dispatch(), marcandre . lureau, 2016/08/10
[Qemu-devel] [PATCH v4 15/17] build-sys: remove qmp-commands-old.h, marcandre . lureau, 2016/08/10
[Qemu-devel] [PATCH v4 17/17] qmp-commands.txt: fix some styling, marcandre . lureau, 2016/08/10
[Qemu-devel] [PATCH v4 16/17] Replace qmp-commands.hx by doc/qmp-commands.txt, marcandre . lureau, 2016/08/10
Re: [Qemu-devel] [PATCH v4 00/17] qapi: remove the 'middle' mode, no-reply, 2016/08/11