From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46446) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbQIJ-0004jk-Lj for qemu-devel@nongnu.org; Fri, 27 Mar 2015 05:11:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbQIF-0002LM-Gw for qemu-devel@nongnu.org; Fri, 27 Mar 2015 05:11:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35170) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbQIF-0002LH-9y for qemu-devel@nongnu.org; Fri, 27 Mar 2015 05:11:07 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-23-git-send-email-eblake@redhat.com> Date: Fri, 27 Mar 2015 10:11:02 +0100 In-Reply-To: <1427227433-5030-23-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:47 -0600") Message-ID: <871tkau7o9.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 22/28] qapi: Whitelist commands that don't return dictionary List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Eric Blake writes: > ...or an array of dictionaries. Although we have to cater to > existing commands, returning a non-dictionary means the command > is not extensible (no new name/value pairs can be added if more > information must be returned in parallel). By making the > whitelist explicit, any new command that falls foul of this > practice will have to be self-documenting, which will encourage > developers to either justify the action or rework the design to > use a dictionary after all. > > Signed-off-by: Eric Blake > --- > scripts/qapi.py | 30 ++++++++++++++++++++++++++++-- > tests/qapi-schema/returns-alternate.err | 1 + > tests/qapi-schema/returns-alternate.exit | 2 +- > tests/qapi-schema/returns-alternate.json | 2 +- > tests/qapi-schema/returns-alternate.out | 4 ---- > tests/qapi-schema/returns-int.json | 3 ++- > tests/qapi-schema/returns-int.out | 2 +- > tests/qapi-schema/returns-whitelist.err | 1 + > tests/qapi-schema/returns-whitelist.exit | 2 +- > tests/qapi-schema/returns-whitelist.json | 2 +- > tests/qapi-schema/returns-whitelist.out | 7 ------- > 11 files changed, 37 insertions(+), 19 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index ed5385a..9421431 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -33,6 +33,30 @@ builtin_types = { > 'size': 'QTYPE_QINT', > } > > +# Whitelist of commands allowed to return a non-dictionary > +returns_whitelist = [ > + # From QMP: > + 'human-monitor-command', > + 'query-migrate-cache-size', > + 'query-tpm-models', > + 'query-tpm-types', > + 'ringbuf-read', > + > + # From QGA: > + 'guest-file-open', > + 'guest-fsfreeze-freeze', > + 'guest-fsfreeze-freeze-list', > + 'guest-fsfreeze-status', > + 'guest-fsfreeze-thaw', > + 'guest-get-time', > + 'guest-set-vcpus', > + 'guest-sync', > + 'guest-sync-delimited', > + > + # From qapi-schema-test: > + 'user_def_cmd3', > +] > + Since there's just one whitelist, all schemata share it, and that means it's too permissive for each of them. Sloppy, but good enough. If the sloppiness bothers us, here are two alternatives: * Program takes the whitelist as argument, say scripts/qapi-commands.py --legacy-returns qmp-legacy-returns ... * Leave enforcing to C If a command 'frobnicate' returns a non-dictionary, generate something like #ifndef FROBNICATE_LEGACY_RETURN_OK #error Command 'frobnicate' should return a dictionary #endif Then manually define the macros necessary to keep the current use working in a suitable header. > enum_types = [] > struct_types = [] > union_types = [] > @@ -350,10 +374,12 @@ def check_command(expr, expr_info): > check_type(expr_info, "'data' for command '%s'" % name, > expr.get('data'), > allowed_metas=['union', 'struct'], allow_optional=True) > + returns_meta = ['union', 'struct'] > + if name in returns_whitelist: > + returns_meta += ['built-in', 'alternate', 'enum'] > check_type(expr_info, "'returns' for command '%s'" % name, > expr.get('returns'), allow_array=True, > - allowed_metas=['built-in', 'union', 'alternate', 'struct', > - 'enum'], allow_optional=True) > + allowed_metas=returns_meta, allow_optional=True) > > def check_event(expr, expr_info): > global events [...] Reviewed-by: Markus Armbruster