From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32788) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ar28Z-0006gH-MZ for qemu-devel@nongnu.org; Fri, 15 Apr 2016 07:42:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ar28U-0003Z9-LW for qemu-devel@nongnu.org; Fri, 15 Apr 2016 07:42:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34656) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ar28U-0003Z4-E3 for qemu-devel@nongnu.org; Fri, 15 Apr 2016 07:42:06 -0400 From: Markus Armbruster References: <1460131992-32278-1-git-send-email-eblake@redhat.com> <1460131992-32278-16-git-send-email-eblake@redhat.com> Date: Fri, 15 Apr 2016 13:42:02 +0200 In-Reply-To: <1460131992-32278-16-git-send-email-eblake@redhat.com> (Eric Blake's message of "Fri, 8 Apr 2016 10:13:08 -0600") Message-ID: <87shynt7xx.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v14 15/19] qapi-commands: Wrap argument visit in visit_start_struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake 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 > > --- > 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.