marcandre.lur...@redhat.com writes:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> 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?
> 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 <marcandre.lur...@redhat.com>
> ---
> 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.
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('''
}
