Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>
> ---
> 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.
