Hi On Wed, Aug 17, 2016 at 6:49 PM Markus Armbruster <[email protected]> wrote:
> [email protected] writes: > > > From: Marc-André Lureau <[email protected]> > > > > 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 <[email protected]> > > --- > > 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
