From patchwork Wed Apr 29 13:06:41 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 466007 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4A6471402BD for ; Wed, 29 Apr 2015 23:12:57 +1000 (AEST) Received: from localhost ([::1]:38825 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRnK-0000Qv-MF for incoming@patchwork.ozlabs.org; Wed, 29 Apr 2015 09:12:54 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58558) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRhr-0007o1-9B for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnRhp-00012e-0o for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRho-00012E-HC for qemu-devel@nongnu.org; Wed, 29 Apr 2015 09:07:12 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 46B408E70F; Wed, 29 Apr 2015 13:07:12 +0000 (UTC) Received: from red.redhat.com (ovpn-113-25.phx2.redhat.com [10.3.113.25]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3TD6ww5012100; Wed, 29 Apr 2015 09:07:11 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Wed, 29 Apr 2015 07:06:41 -0600 Message-Id: <1430312814-19706-27-git-send-email-eblake@redhat.com> In-Reply-To: <1430312814-19706-1-git-send-email-eblake@redhat.com> References: <1430312814-19706-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, berto@igalia.com, armbru@redhat.com Subject: [Qemu-devel] [PATCH v7 26/39] qapi: Whitelist commands that don't return dictionary X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org ...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. It's a little bit sloppy that we share a single whitelist among three clients (it's too permissive for each). If this is a problem, a future patch could tighten things by having the generator take the whitelist as an argument (as in scripts/qapi-commands.py --legacy-returns=...), or by having the generator output C code that requires explicit use of the whitelist (as in: #ifndef FROBNICATE_LEGACY_RETURN_OK # error Command 'frobnicate' should return a dictionary #endif then having the callers define appropriate macros). But until we need such fine-grained separation (if ever), this patch does the job just fine. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster --- scripts/qapi.py | 31 ++++++++++++++++++++++++++++--- 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(+), 20 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index c6ab8f3..2b52932 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -32,6 +32,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', +] + enum_types = [] struct_types = [] union_types = [] @@ -354,11 +378,12 @@ def check_command(expr, expr_info): check_type(expr_info, "'data' for command '%s'" % name, expr.get('data'), allow_dict=True, allow_optional=True, allow_metas=['union', 'struct']) + 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, allow_dict=True, - allow_optional=True, - allow_metas=['built-in', 'union', 'alternate', 'struct', - 'enum']) + allow_optional=True, allow_metas=returns_meta) def check_event(expr, expr_info): global events diff --git a/tests/qapi-schema/returns-alternate.err b/tests/qapi-schema/returns-alternate.err index e69de29..dfbb419 100644 --- a/tests/qapi-schema/returns-alternate.err +++ b/tests/qapi-schema/returns-alternate.err @@ -0,0 +1 @@ +tests/qapi-schema/returns-alternate.json:3: 'returns' for command 'oops' cannot use alternate type 'Alt' diff --git a/tests/qapi-schema/returns-alternate.exit b/tests/qapi-schema/returns-alternate.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/returns-alternate.exit +++ b/tests/qapi-schema/returns-alternate.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/returns-alternate.json b/tests/qapi-schema/returns-alternate.json index b3b91fd..972390c 100644 --- a/tests/qapi-schema/returns-alternate.json +++ b/tests/qapi-schema/returns-alternate.json @@ -1,3 +1,3 @@ -# FIXME: we should reject returns if it is an alternate type +# we reject returns if it is an alternate type { 'alternate': 'Alt', 'data': { 'a': 'int', 'b': 'str' } } { 'command': 'oops', 'returns': 'Alt' } diff --git a/tests/qapi-schema/returns-alternate.out b/tests/qapi-schema/returns-alternate.out index 8a03ed3..e69de29 100644 --- a/tests/qapi-schema/returns-alternate.out +++ b/tests/qapi-schema/returns-alternate.out @@ -1,4 +0,0 @@ -[OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('a', 'int'), ('b', 'str')]))]), - OrderedDict([('command', 'oops'), ('returns', 'Alt')])] -[{'enum_name': 'AltKind', 'enum_values': None}] -[] diff --git a/tests/qapi-schema/returns-int.json b/tests/qapi-schema/returns-int.json index 7888fb1..870ec63 100644 --- a/tests/qapi-schema/returns-int.json +++ b/tests/qapi-schema/returns-int.json @@ -1,2 +1,3 @@ # It is okay (although not extensible) to return a non-dictionary -{ 'command': 'okay', 'returns': 'int' } +# But to make it work, the name must be in a whitelist +{ 'command': 'guest-get-time', 'returns': 'int' } diff --git a/tests/qapi-schema/returns-int.out b/tests/qapi-schema/returns-int.out index 36b00a9..70b3ac5 100644 --- a/tests/qapi-schema/returns-int.out +++ b/tests/qapi-schema/returns-int.out @@ -1,3 +1,3 @@ -[OrderedDict([('command', 'okay'), ('returns', 'int')])] +[OrderedDict([('command', 'guest-get-time'), ('returns', 'int')])] [] [] diff --git a/tests/qapi-schema/returns-whitelist.err b/tests/qapi-schema/returns-whitelist.err index e69de29..a41f019 100644 --- a/tests/qapi-schema/returns-whitelist.err +++ b/tests/qapi-schema/returns-whitelist.err @@ -0,0 +1 @@ +tests/qapi-schema/returns-whitelist.json:10: 'returns' for command 'no-way-this-will-get-whitelisted' cannot use built-in type 'array of int' diff --git a/tests/qapi-schema/returns-whitelist.exit b/tests/qapi-schema/returns-whitelist.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/returns-whitelist.exit +++ b/tests/qapi-schema/returns-whitelist.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/returns-whitelist.json b/tests/qapi-schema/returns-whitelist.json index 8328563..e8b3cea 100644 --- a/tests/qapi-schema/returns-whitelist.json +++ b/tests/qapi-schema/returns-whitelist.json @@ -1,4 +1,4 @@ -# FIXME: we should enforce that 'returns' be a dict or array of dict unless whitelisted +# we enforce that 'returns' be a dict or array of dict unless whitelisted { 'command': 'human-monitor-command', 'data': {'command-line': 'str', '*cpu-index': 'int'}, 'returns': 'str' } diff --git a/tests/qapi-schema/returns-whitelist.out b/tests/qapi-schema/returns-whitelist.out index 2adcd8b..e69de29 100644 --- a/tests/qapi-schema/returns-whitelist.out +++ b/tests/qapi-schema/returns-whitelist.out @@ -1,7 +0,0 @@ -[OrderedDict([('command', 'human-monitor-command'), ('data', OrderedDict([('command-line', 'str'), ('*cpu-index', 'int')])), ('returns', 'str')]), - OrderedDict([('enum', 'TpmModel'), ('data', ['tpm-tis'])]), - OrderedDict([('command', 'query-tpm-models'), ('returns', ['TpmModel'])]), - OrderedDict([('command', 'guest-get-time'), ('returns', 'int')]), - OrderedDict([('command', 'no-way-this-will-get-whitelisted'), ('returns', ['int'])])] -[{'enum_name': 'TpmModel', 'enum_values': ['tpm-tis']}] -[]