[3/8] qapi script: check correctness of discriminator values in union

Submitted by Wayne Xia on Nov. 6, 2013, 7:33 p.m.

Details

Message ID 1383766420-20745-4-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia Nov. 6, 2013, 7:33 p.m.
It will check whether the values specified are written correctly when
discriminator is a pre-defined enum type, which help check whether the
schema is in good form.

It is allowed, that not every value in enum is used, so does not check
that case.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 scripts/qapi-visit.py |   11 +++++++++++
 scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

Comments

Eric Blake Nov. 12, 2013, 6:12 p.m.
On 11/06/2013 12:33 PM, Wenchao Xia wrote:
> It will check whether the values specified are written correctly when
> discriminator is a pre-defined enum type, which help check whether the
> schema is in good form.
> 
> It is allowed, that not every value in enum is used, so does not check
> that case.

Again, I think you should require that every value in the enum is used.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  scripts/qapi-visit.py |   11 +++++++++++
>  scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index b3d3af8..612dc4d 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -251,6 +251,17 @@ def generate_visit_union(expr):
>          assert not base
>          return generate_visit_anon_union(name, members)
>  
> +    # If discriminator is specified and it is a pre-defined enum in schema,
> +    # check its correctness
> +    enum_define = discriminator_find_enum_define(expr)
> +    if enum_define:
> +        for key in members:
> +            if not key in enum_define["enum_values"]:
> +                sys.stderr.write("Discriminator value '%s' not found in "
> +                                 "enum '%s'\n" %
> +                                 (key, enum_define["enum_name"]))
> +                sys.exit(1)

This checks for union branches not covered by the enum, but does not
check for duplicate union branches, nor does it check for enum values
not covered by a union branch.
Wayne Xia Nov. 13, 2013, 2:30 a.m.
于 2013/11/13 2:12, Eric Blake 写道:
> On 11/06/2013 12:33 PM, Wenchao Xia wrote:
>> It will check whether the values specified are written correctly when
>> discriminator is a pre-defined enum type, which help check whether the
>> schema is in good form.
>>
>> It is allowed, that not every value in enum is used, so does not check
>> that case.
>
> Again, I think you should require that every value in the enum is used.
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   scripts/qapi-visit.py |   11 +++++++++++
>>   scripts/qapi.py       |   33 +++++++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+), 0 deletions(-)
>>
>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
>> index b3d3af8..612dc4d 100644
>> --- a/scripts/qapi-visit.py
>> +++ b/scripts/qapi-visit.py
>> @@ -251,6 +251,17 @@ def generate_visit_union(expr):
>>           assert not base
>>           return generate_visit_anon_union(name, members)
>>
>> +    # If discriminator is specified and it is a pre-defined enum in schema,
>> +    # check its correctness
>> +    enum_define = discriminator_find_enum_define(expr)
>> +    if enum_define:
>> +        for key in members:
>> +            if not key in enum_define["enum_values"]:
>> +                sys.stderr.write("Discriminator value '%s' not found in "
>> +                                 "enum '%s'\n" %
>> +                                 (key, enum_define["enum_name"]))
>> +                sys.exit(1)
>
> This checks for union branches not covered by the enum, but does not
> check for duplicate union branches, nor does it check for enum values
> not covered by a union branch.
>
   I agree that, allowing enum values not covered by union branch,
require more carefully error handling for caller, such as visitors,
will add the check for safty reason.

Patch hide | download patch | download mbox

diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index b3d3af8..612dc4d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -251,6 +251,17 @@  def generate_visit_union(expr):
         assert not base
         return generate_visit_anon_union(name, members)
 
+    # If discriminator is specified and it is a pre-defined enum in schema,
+    # check its correctness
+    enum_define = discriminator_find_enum_define(expr)
+    if enum_define:
+        for key in members:
+            if not key in enum_define["enum_values"]:
+                sys.stderr.write("Discriminator value '%s' not found in "
+                                 "enum '%s'\n" %
+                                 (key, enum_define["enum_name"]))
+                sys.exit(1)
+
     ret = generate_visit_enum('%sKind' % name, members.keys())
 
     if base:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 82f586e..f93bda1 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -383,3 +383,36 @@  def guardend(name):
 
 ''',
                  name=guardname(name))
+
+# This function can be used to check whether "base" is valid
+def find_base_fields(base):
+    base_struct_define = find_struct(base)
+    if not base_struct_define:
+        return None
+    return base_struct_define.get('data')
+
+# Return the discriminator enum define, if discriminator is specified in
+# @expr and it is a pre-defined enum type
+def discriminator_find_enum_define(expr):
+    discriminator = expr.get('discriminator')
+    base = expr.get('base')
+
+    # Only support discriminator when base present
+    if not (discriminator and base):
+        return None
+
+    base_fields = find_base_fields(base)
+
+    if not base_fields:
+        sys.stderr.write("Base '%s' is not a valid type\n"
+                         % base)
+        sys.exit(1)
+
+    discriminator_type = base_fields.get(discriminator)
+
+    if not discriminator_type:
+        sys.stderr.write("Discriminator '%s' not found in schema\n"
+                         % discriminator)
+        sys.exit(1)
+
+    return find_enum(discriminator_type)