Re: [Qemu-devel] [PATCH v3 10/15] 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.

Begs the question why this isn't a bug fix :)

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.

What about QGA?

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.

The commit message should explain these things.

> static void qmp_marshal_stop(QDict *args, QObject **ret, Error **errp)
>  {
> +    Visitor *v = NULL;
>      Error *err = NULL;
> -
> -    (void)args;
> +    if (args) {
> +        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;
> +        }
> +    }
>
>      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);
> +    }
>  }

The new code closely resembles code for a command with arguments.
That's a good sign.  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.

I think that's also worth explaining in the commit message.

However, this is not the only change to generated code: marshalers for
commands with arguments also change, like this:

   static void qmp_marshal_netdev_del(QDict *args, QObject **ret, Error **errp)
   {
  +    Visitor *v = NULL;
       Error *err = NULL;
  -    Visitor *v;
       q_obj_netdev_del_arg arg = {0};

       v = qmp_input_visitor_new(QOBJECT(args), true);

The initialization is dead for commands with arguments.  For commands
without arguments, it suppresses a bogus "may be used uninitialized"
warning.  Oh well.

>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  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

You remembered to add a test.  Appreciated!

> @@ -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")));

Matches how the other request objects are built.  qobject_from_json()
could be simpler, though.  Your choice.

> +
> +    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 11aa54b..735e463 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -97,11 +97,27 @@ def gen_marshal_decl(name, export):
>                   proto=gen_marshal_proto(name, export))
>  
>  
> +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, export):
>      ret = mcgen('''
>  
>  %(proto)s
>  {
> +    Visitor *v = NULL;
>      Error *err = NULL;
>  ''',
>                  proto=gen_marshal_proto(name, export))
> @@ -112,17 +128,23 @@ def gen_marshal(name, arg_type, boxed, ret_type, export):
>  ''',
>                       c_type=ret_type.c_type())
>  
> +    visit_members = ''
>      if arg_type and not arg_type.is_empty():
> +        visit_members = mcgen('visit_type_%(c_name)s_members(v, &arg, &err);',
> +                            c_name=arg_type.c_name())
>          ret += mcgen('''
> -    Visitor *v;
>      %(c_name)s arg = {0};
>  
> +''',
> +                     c_name=arg_type.c_name())
> +
> +    ret += if_args(arg_type, lambda: 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

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.

The easiest fix is to say

        visit_members = ('visit_type_%(c_name)s_members(v, &arg, &err);'
                         % arg_type.c_name())

>      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=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

I think this comment becomes wrong, and should be dropped.

> -    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())
> +    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, lambda: 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('''
>  }

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.

It also avoids the dead initialization of v, because it's easy to do.

I'm fine with clever when it saves repetition, but when it doesn't, I'll
have stupid, please.


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 = {0};
 
+''',
+                     c_name=arg_type.c_name())
+        visit_members = ('visit_type_%s_members(v, &arg, &err);'
+                         % arg_type.c_name())
+    else:
+        ret += mcgen('''
+    Visitor *v = NULL;
+
+    if (args) {
+''')
+        push_indent();
+        visit_members = ''
+
+    ret += 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);
     }
@@ -131,35 +145,39 @@ def gen_marshal(name, arg_type, boxed, ret_type, export):
         goto out;
     }
 ''',
-                     c_name=arg_type.c_name())
-
-    else:
+                 visit_members=visit_members)
+    if not arg_type or arg_type.is_empty():
+        pop_indent()
         ret += mcgen('''
-
-    (void)args;
+    }
 ''')
 
     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('''
+    ret += mcgen('''
 
 out:
-''')
-    ret += mcgen('''
     error_propagate(errp, err);
 ''')
-    if arg_type and not arg_type.is_empty():
+    if not arg_type or arg_type.is_empty():
         ret += mcgen('''
+    if (args) {
+''')
+        push_indent();
+    ret += mcgen('''
     visit_free(v);
     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)
+    if not arg_type or arg_type.is_empty():
+        pop_indent()
+        ret += mcgen('''
+    }
+''')
 
     ret += mcgen('''
 }