Re: [Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in visit_start_struct

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The qmp-input visitor was playing rather fast and loose: when

I guess (some of) its *users* are playing fast and loose, and the
visitor itself lets them.  The patch's title suggests "some of its
users" == qapi-commands.py.

> visiting a QDict, you could grab members of the root dictionary
> without first pushing into the dict.  But we are about to tighten
> the input visitor, at which point the generated marshal code
> MUST follow the same paradigms as everyone else, of pushing into
> the struct before grabbing its keys, because the value of 'name'
> should be ignored on the top-level visit.
>
> Generated code grows as follows:
>
> |@@ -515,7 +695,15 @@ void qmp_marshal_blockdev_backup(QDict *
> |     BlockdevBackup arg = {0};
> |
> |     v = qmp_input_get_visitor(qiv);
> |+    visit_start_struct(v, NULL, NULL, 0, &err);
> |+    if (err) {
> |+        goto out;
> |+    }
> |     visit_type_BlockdevBackup_members(v, &arg, &err);
> |+    if (!err) {
> |+        visit_check_struct(v, &err);
> |+    }

Does this find errors that weren't found before?

> |+    visit_end_struct(v);
> |     if (err) {
> |         goto out;
> |     }
> |@@ -527,7 +715,9 @@ out:
> |     qmp_input_visitor_cleanup(qiv);
> |     qdv = qapi_dealloc_visitor_new();
> |     v = qapi_dealloc_get_visitor(qdv);
> |+    visit_start_struct(v, NULL, NULL, 0, NULL);
> |     visit_type_BlockdevBackup_members(v, &arg, NULL);
> |+    visit_end_struct(v);
> |     qapi_dealloc_visitor_cleanup(qdv);
> | }
>
> Note that this change could also make it possible for the
> marshalling code to automatically detect excess input at the top
> level, and not just in nested dictionaries.  However, that checking
> is not currently useful (and we rely on the manual checking in
> monitor.c:qmp_check_client_args() instead) as long as qmp-commands.hx
> uses .args_type, and as long as we have 'name:O' as an arg-type that
> explicitly allows unknown top-level keys because we haven't yet
> converted 'device_add' and 'netdev_add' to introspectible use of
> 'any'.

Hmm, that explains why finding these additional errors wouldn't be
useful.  Good to know, but doesn't quite answer my question.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v14: rebase to master context
> v13: rebase to earlier patches
> v12: new patch
> ---
>  scripts/qapi-commands.py | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index b570069..9c1bd25 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -121,7 +121,15 @@ def gen_marshal(name, arg_type, ret_type):
>      %(c_name)s arg = {0};
>
>      v = qmp_input_get_visitor(qiv);
> +    visit_start_struct(v, NULL, NULL, 0, &err);
> +    if (err) {
> +        goto out;
> +    }
>      visit_type_%(c_name)s_members(v, &arg, &err);
> +    if (!err) {
> +        visit_check_struct(v, &err);
> +    }
> +    visit_end_struct(v);
>      if (err) {
>          goto out;
>      }
> @@ -150,7 +158,9 @@ out:
>      qmp_input_visitor_cleanup(qiv);
>      qdv = qapi_dealloc_visitor_new();
>      v = qapi_dealloc_get_visitor(qdv);
> +    visit_start_struct(v, NULL, NULL, 0, NULL);
>      visit_type_%(c_name)s_members(v, &arg, NULL);
> +    visit_end_struct(v);
>      qapi_dealloc_visitor_cleanup(qdv);
>  ''',
>                       c_name=arg_type.c_name())

No visit_check_struct() here.  I think its contract should explicitly
state that you may omit it when you're not interested in errors.