| Message ID | 1460131992-32278-16-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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 <eblake@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.
On 04/15/2016 05:42 AM, Markus Armbruster wrote: > Eric Blake <eblake@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? All that this could find is excess input, but we are already checking for excess input prior to calling into the generated marshaling code, so it doesn't find anything new. >> >> 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. I guess what it really translates to is we are now doing redundant checking, and I should do a followup patch to simplify monitor.c. >> @@ -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. Indeed, calling visit_check_struct(, NULL) can't report any errors, so skipping it should be documented as safe.
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())
The qmp-input visitor was playing rather fast and loose: when 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); |+ } |+ 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'. Signed-off-by: Eric Blake <eblake@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(+)