From patchwork Tue Mar 24 20:03:30 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 454028 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 5CAAD14009B for ; Wed, 25 Mar 2015 07:20:31 +1100 (AEDT) Received: from localhost ([::1]:34336 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaVJN-0001K5-A9 for incoming@patchwork.ozlabs.org; Tue, 24 Mar 2015 16:20:29 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43495) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaV3f-0000Qy-FW for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaV3Y-00043c-F7 for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35841) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaV3Y-00043G-8F for qemu-devel@nongnu.org; Tue, 24 Mar 2015 16:04:08 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t2OK45jx010844 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 24 Mar 2015 16:04:06 -0400 Received: from red.redhat.com (ovpn-113-74.phx2.redhat.com [10.3.113.74]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t2OK3x7S028300; Tue, 24 Mar 2015 16:04:04 -0400 From: Eric Blake To: qemu-devel@nongnu.org Date: Tue, 24 Mar 2015 14:03:30 -0600 Message-Id: <1427227433-5030-6-git-send-email-eblake@redhat.com> In-Reply-To: <1427227433-5030-1-git-send-email-eblake@redhat.com> References: <1427227433-5030-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, wenchaoqemu@gmail.com, lcapitulino@redhat.com Subject: [Qemu-devel] [PATCH v5 05/28] qapi: Better error messages for bad enums 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 The previous commit demonstrated that the generator had several flaws with less-than-perfect enums: - an enum that listed the same string twice (or two variant strings that map to the same C enumerator) ended up generating an invalid C enum - because the generator adds a _MAX terminator to each enum, the use of an enum member 'max' can also cause this clash - if an enum omits 'data', the generator left a python stack trace rather than a graceful message - an enum that used a non-array 'data' was silently accepted by the parser - an enum that used non-string members in the 'data' member was silently accepted by the parser Add check_enum to cover these situations, and update testcases to match. While valid .json files won't trigger any of these cases, we might as well be nicer to developers that make a typo while trying to add new QAPI code. Signed-off-by: Eric Blake Reviewed-by: Markus Armbruster --- scripts/qapi.py | 34 +++++++++++++++++++++++++++----- tests/qapi-schema/enum-clash-member.err | 1 + tests/qapi-schema/enum-clash-member.exit | 2 +- tests/qapi-schema/enum-clash-member.json | 2 +- tests/qapi-schema/enum-clash-member.out | 3 --- tests/qapi-schema/enum-dict-member.err | 1 + tests/qapi-schema/enum-dict-member.exit | 2 +- tests/qapi-schema/enum-dict-member.json | 2 +- tests/qapi-schema/enum-dict-member.out | 3 --- tests/qapi-schema/enum-max-member.err | 1 + tests/qapi-schema/enum-max-member.exit | 2 +- tests/qapi-schema/enum-max-member.json | 4 ++-- tests/qapi-schema/enum-max-member.out | 3 --- tests/qapi-schema/enum-missing-data.err | 7 +------ tests/qapi-schema/enum-missing-data.json | 2 +- tests/qapi-schema/enum-wrong-data.err | 1 + tests/qapi-schema/enum-wrong-data.exit | 2 +- tests/qapi-schema/enum-wrong-data.json | 2 +- tests/qapi-schema/enum-wrong-data.out | 3 --- 19 files changed, 44 insertions(+), 33 deletions(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index 20ee505..3ce8c33 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -311,13 +311,37 @@ def check_union(expr, expr_info): # Todo: add checking for values. Key is checked as above, value can be # also checked here, but we need more functions to handle array case. +def check_enum(expr, expr_info): + name = expr['enum'] + members = expr.get('data') + values = { 'MAX': '(automatic)' } + + if not isinstance(members, list): + raise QAPIExprError(expr_info, + "Enum '%s' requires an array for 'data'" % name) + for member in members: + if not isinstance(member, str): + raise QAPIExprError(expr_info, + "Enum '%s' member '%s' is not a string" + % (name, member)) + key = _generate_enum_string(member) + if key in values: + raise QAPIExprError(expr_info, + "Enum '%s' member '%s' clashes with '%s'" + % (name, member, values[key])) + values[key] = member + def check_exprs(schema): for expr_elem in schema.exprs: expr = expr_elem['expr'] - if expr.has_key('union'): - check_union(expr, expr_elem['info']) - if expr.has_key('event'): - check_event(expr, expr_elem['info']) + info = expr_elem['info'] + + if expr.has_key('enum'): + check_enum(expr, info) + elif expr.has_key('union'): + check_union(expr, info) + elif expr.has_key('event'): + check_event(expr, info) def parse_schema(input_file): try: @@ -331,7 +355,7 @@ def parse_schema(input_file): for expr_elem in schema.exprs: expr = expr_elem['expr'] if expr.has_key('enum'): - add_enum(expr['enum'], expr['data']) + add_enum(expr['enum'], expr.get('data')) elif expr.has_key('union'): add_union(expr) elif expr.has_key('type'): diff --git a/tests/qapi-schema/enum-clash-member.err b/tests/qapi-schema/enum-clash-member.err index e69de29..48bd136 100644 --- a/tests/qapi-schema/enum-clash-member.err +++ b/tests/qapi-schema/enum-clash-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-clash-member.json:2: Enum 'MyEnum' member 'ONE' clashes with 'one' diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-schema/enum-clash-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-clash-member.exit +++ b/tests/qapi-schema/enum-clash-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-schema/enum-clash-member.json index 99d442a..b7dc02a 100644 --- a/tests/qapi-schema/enum-clash-member.json +++ b/tests/qapi-schema/enum-clash-member.json @@ -1,2 +1,2 @@ -# FIXME: we should reject enums where members will clash when mapped to C enum +# we reject enums where members will clash when mapped to C enum { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } diff --git a/tests/qapi-schema/enum-clash-member.out b/tests/qapi-schema/enum-clash-member.out index 0814459..e69de29 100644 --- a/tests/qapi-schema/enum-clash-member.out +++ b/tests/qapi-schema/enum-clash-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', ['one', 'ONE'])])] -[{'enum_name': 'MyEnum', 'enum_values': ['one', 'ONE']}] -[] diff --git a/tests/qapi-schema/enum-dict-member.err b/tests/qapi-schema/enum-dict-member.err index e69de29..7e966a8 100644 --- a/tests/qapi-schema/enum-dict-member.err +++ b/tests/qapi-schema/enum-dict-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-dict-member.json:2: Enum 'MyEnum' member 'OrderedDict([('value', 'str')])' is not a string diff --git a/tests/qapi-schema/enum-dict-member.exit b/tests/qapi-schema/enum-dict-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-dict-member.exit +++ b/tests/qapi-schema/enum-dict-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-dict-member.json b/tests/qapi-schema/enum-dict-member.json index de4d6bf..79672e0 100644 --- a/tests/qapi-schema/enum-dict-member.json +++ b/tests/qapi-schema/enum-dict-member.json @@ -1,2 +1,2 @@ -# FIXME: we should reject any enum member that is not a string +# we reject any enum member that is not a string { 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] } diff --git a/tests/qapi-schema/enum-dict-member.out b/tests/qapi-schema/enum-dict-member.out index 8b293f8..e69de29 100644 --- a/tests/qapi-schema/enum-dict-member.out +++ b/tests/qapi-schema/enum-dict-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', [OrderedDict([('value', 'str')])])])] -[{'enum_name': 'MyEnum', 'enum_values': [OrderedDict([('value', 'str')])]}] -[] diff --git a/tests/qapi-schema/enum-max-member.err b/tests/qapi-schema/enum-max-member.err index e69de29..f77837f 100644 --- a/tests/qapi-schema/enum-max-member.err +++ b/tests/qapi-schema/enum-max-member.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-max-member.json:3: Enum 'MyEnum' member 'max' clashes with '(automatic)' diff --git a/tests/qapi-schema/enum-max-member.exit b/tests/qapi-schema/enum-max-member.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-max-member.exit +++ b/tests/qapi-schema/enum-max-member.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-max-member.json b/tests/qapi-schema/enum-max-member.json index ea854c4..6af4662 100644 --- a/tests/qapi-schema/enum-max-member.json +++ b/tests/qapi-schema/enum-max-member.json @@ -1,3 +1,3 @@ -# FIXME: we should either reject user-supplied 'max', or munge the implicit -# max value we generate at the end of an array +# we reject user-supplied 'max' for clashing with implicit enum end +# FIXME: should we instead munge the the implicit value to avoid the clash? { 'enum': 'MyEnum', 'data': [ 'max' ] } diff --git a/tests/qapi-schema/enum-max-member.out b/tests/qapi-schema/enum-max-member.out index c933044..e69de29 100644 --- a/tests/qapi-schema/enum-max-member.out +++ b/tests/qapi-schema/enum-max-member.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', ['max'])])] -[{'enum_name': 'MyEnum', 'enum_values': ['max']}] -[] diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err index 814ab26..b8ccae0 100644 --- a/tests/qapi-schema/enum-missing-data.err +++ b/tests/qapi-schema/enum-missing-data.err @@ -1,6 +1 @@ -Traceback (most recent call last): - File "tests/qapi-schema/test-qapi.py", line 19, in - exprs = parse_schema(sys.argv[1]) - File "scripts/qapi.py", line 334, in parse_schema - add_enum(expr['enum'], expr['data']) -KeyError: 'data' +tests/qapi-schema/enum-missing-data.json:2: Enum 'MyEnum' requires an array for 'data' diff --git a/tests/qapi-schema/enum-missing-data.json b/tests/qapi-schema/enum-missing-data.json index 01f3f32..558fd35 100644 --- a/tests/qapi-schema/enum-missing-data.json +++ b/tests/qapi-schema/enum-missing-data.json @@ -1,2 +1,2 @@ -# FIXME: we should require that all QAPI enums have a data array +# we require that all QAPI enums have a data array { 'enum': 'MyEnum' } diff --git a/tests/qapi-schema/enum-wrong-data.err b/tests/qapi-schema/enum-wrong-data.err index e69de29..11b4347 100644 --- a/tests/qapi-schema/enum-wrong-data.err +++ b/tests/qapi-schema/enum-wrong-data.err @@ -0,0 +1 @@ +tests/qapi-schema/enum-wrong-data.json:2: Enum 'MyEnum' requires an array for 'data' diff --git a/tests/qapi-schema/enum-wrong-data.exit b/tests/qapi-schema/enum-wrong-data.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/enum-wrong-data.exit +++ b/tests/qapi-schema/enum-wrong-data.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/enum-wrong-data.json b/tests/qapi-schema/enum-wrong-data.json index 61d25ec..7b3e255 100644 --- a/tests/qapi-schema/enum-wrong-data.json +++ b/tests/qapi-schema/enum-wrong-data.json @@ -1,2 +1,2 @@ -# FIXME: we should require that all qapi enums have an array for data +# we require that all qapi enums have an array for data { 'enum': 'MyEnum', 'data': { 'value': 'str' } } diff --git a/tests/qapi-schema/enum-wrong-data.out b/tests/qapi-schema/enum-wrong-data.out index 28d2211..e69de29 100644 --- a/tests/qapi-schema/enum-wrong-data.out +++ b/tests/qapi-schema/enum-wrong-data.out @@ -1,3 +0,0 @@ -[OrderedDict([('enum', 'MyEnum'), ('data', OrderedDict([('value', 'str')]))])] -[{'enum_name': 'MyEnum', 'enum_values': OrderedDict([('value', 'str')])}] -[]