Re: [Qemu-devel] [PATCH v4 12/17] qapi: check invalid arguments on no-args commands

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@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.lureau@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('''
 }