diff mbox series

[20/25] qapi: Improve reporting of missing / unknown definition keys

Message ID 20190924132830.15835-21-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: Pay back some frontend technical debt | expand

Commit Message

Markus Armbruster Sept. 24, 2019, 1:28 p.m. UTC 
Have check_exprs() call check_keys() later, so its error messages gain
an "in definition" line.

Both check_keys() and check_name_is_str() check the definition's name
is a string.  Since check_keys() now runs after check_name_is_str()
rather than before, its check is dead.  Bury it.  Checking values in
check_keys() is unclean anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/common.py                  | 40 ++++++++++++-------------
 tests/qapi-schema/alternate-base.err    |  1 +
 tests/qapi-schema/bad-type-bool.err     |  2 +-
 tests/qapi-schema/bad-type-dict.err     |  2 +-
 tests/qapi-schema/double-type.err       |  1 +
 tests/qapi-schema/enum-missing-data.err |  1 +
 tests/qapi-schema/unknown-expr-key.err  |  1 +
 7 files changed, 25 insertions(+), 23 deletions(-)

Comments

Eric Blake Sept. 24, 2019, 6:13 p.m. UTC | #1 
On 9/24/19 8:28 AM, Markus Armbruster wrote:
> Have check_exprs() call check_keys() later, so its error messages gain
> an "in definition" line.
> 
> Both check_keys() and check_name_is_str() check the definition's name
> is a string.  Since check_keys() now runs after check_name_is_str()
> rather than before, its check is dead.  Bury it.  Checking values in
> check_keys() is unclean anyway.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/scripts/qapi/common.py
> @@ -905,8 +905,6 @@ def check_known_keys(value, info, source, required, optional):
>  
>  def check_keys(expr, info, meta, required, optional=[]):
>      name = expr[meta]
> -    if not isinstance(name, str):
> -        raise QAPISemError(info, "'%s' key must have a string value" % meta)

Should this be replaced with an assert?  But I'm also okay just dropping
it, since our testsuite shows that we still flag the problems that this
message was originally used for.

Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Sept. 24, 2019, 8:46 p.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 9/24/19 8:28 AM, Markus Armbruster wrote:
>> Have check_exprs() call check_keys() later, so its error messages gain
>> an "in definition" line.
>> 
>> Both check_keys() and check_name_is_str() check the definition's name
>> is a string.  Since check_keys() now runs after check_name_is_str()
>> rather than before, its check is dead.  Bury it.  Checking values in
>> check_keys() is unclean anyway.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/scripts/qapi/common.py
>> @@ -905,8 +905,6 @@ def check_known_keys(value, info, source, required, optional):
>>  
>>  def check_keys(expr, info, meta, required, optional=[]):
>>      name = expr[meta]
>> -    if not isinstance(name, str):
>> -        raise QAPISemError(info, "'%s' key must have a string value" % meta)
>
> Should this be replaced with an assert?  But I'm also okay just dropping
> it, since our testsuite shows that we still flag the problems that this
> message was originally used for.

I'd prefer not to assert, because as of this patch, check_keys() *only*
checks keys, just like its name suggests.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!
diff mbox series

Patch

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 4bfc007833..297b82db4c 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -905,8 +905,6 @@  def check_known_keys(value, info, source, required, optional):
 
 def check_keys(expr, info, meta, required, optional=[]):
     name = expr[meta]
-    if not isinstance(name, str):
-        raise QAPISemError(info, "'%s' key must have a string value" % meta)
     required = required + [meta]
     source = "%s '%s'" % (meta, name)
     check_known_keys(expr, info, source, required, optional)
@@ -964,37 +962,18 @@  def check_exprs(exprs):
 
         if 'enum' in expr:
             meta = 'enum'
-            check_keys(expr, info, 'enum', ['data'], ['if', 'prefix'])
-            normalize_enum(expr)
         elif 'union' in expr:
             meta = 'union'
-            check_keys(expr, info, 'union', ['data'],
-                       ['base', 'discriminator', 'if'])
-            normalize_members(expr.get('base'))
-            normalize_members(expr['data'])
         elif 'alternate' in expr:
             meta = 'alternate'
-            check_keys(expr, info, 'alternate', ['data'], ['if'])
-            normalize_members(expr['data'])
         elif 'struct' in expr:
             meta = 'struct'
-            check_keys(expr, info, 'struct', ['data'],
-                       ['base', 'if', 'features'])
-            normalize_members(expr['data'])
-            normalize_features(expr.get('features'))
         elif 'command' in expr:
             meta = 'command'
-            check_keys(expr, info, 'command', [],
-                       ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob', 'allow-preconfig', 'if'])
-            normalize_members(expr.get('data'))
         elif 'event' in expr:
             meta = 'event'
-            check_keys(expr, info, 'event', [], ['data', 'boxed', 'if'])
-            normalize_members(expr.get('data'))
         else:
             raise QAPISemError(info, "expression is missing metatype")
-        normalize_if(expr)
 
         name = expr[meta]
         check_name_is_str(name, info, "'%s'" % meta)
@@ -1008,20 +987,39 @@  def check_exprs(exprs):
                 % (name, doc.symbol))
 
         if meta == 'enum':
+            check_keys(expr, info, 'enum', ['data'], ['if', 'prefix'])
+            normalize_enum(expr)
             check_enum(expr, info)
         elif meta == 'union':
+            check_keys(expr, info, 'union', ['data'],
+                       ['base', 'discriminator', 'if'])
+            normalize_members(expr.get('base'))
+            normalize_members(expr['data'])
             check_union(expr, info)
         elif meta == 'alternate':
+            check_keys(expr, info, 'alternate', ['data'], ['if'])
+            normalize_members(expr['data'])
             check_alternate(expr, info)
         elif meta == 'struct':
+            check_keys(expr, info, 'struct', ['data'],
+                       ['base', 'if', 'features'])
+            normalize_members(expr['data'])
+            normalize_features(expr.get('features'))
             check_struct(expr, info)
         elif meta == 'command':
+            check_keys(expr, info, 'command', [],
+                       ['data', 'returns', 'gen', 'success-response',
+                        'boxed', 'allow-oob', 'allow-preconfig', 'if'])
+            normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
+            check_keys(expr, info, 'event', [], ['data', 'boxed', 'if'])
+            normalize_members(expr.get('data'))
             check_event(expr, info)
         else:
             assert False, 'unexpected meta type'
 
+        normalize_if(expr)
         check_if(expr, info)
         check_flags(expr, info)
 
diff --git a/tests/qapi-schema/alternate-base.err b/tests/qapi-schema/alternate-base.err
index 4c9158db02..6290665ac2 100644
--- a/tests/qapi-schema/alternate-base.err
+++ b/tests/qapi-schema/alternate-base.err
@@ -1,2 +1,3 @@ 
+tests/qapi-schema/alternate-base.json: In alternate 'Alt':
 tests/qapi-schema/alternate-base.json:4: unknown key 'base' in alternate 'Alt'
 Valid keys are 'alternate', 'data', 'if'.
diff --git a/tests/qapi-schema/bad-type-bool.err b/tests/qapi-schema/bad-type-bool.err
index 62fd70baaf..984a77c4e3 100644
--- a/tests/qapi-schema/bad-type-bool.err
+++ b/tests/qapi-schema/bad-type-bool.err
@@ -1 +1 @@ 
-tests/qapi-schema/bad-type-bool.json:2: 'struct' key must have a string value
+tests/qapi-schema/bad-type-bool.json:2: 'struct' requires a string name
diff --git a/tests/qapi-schema/bad-type-dict.err b/tests/qapi-schema/bad-type-dict.err
index 0b2a2aeac4..e83b8cfb41 100644
--- a/tests/qapi-schema/bad-type-dict.err
+++ b/tests/qapi-schema/bad-type-dict.err
@@ -1 +1 @@ 
-tests/qapi-schema/bad-type-dict.json:2: 'command' key must have a string value
+tests/qapi-schema/bad-type-dict.json:2: 'command' requires a string name
diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
index 44a9dfdd55..ddb22af638 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -1,2 +1,3 @@ 
+tests/qapi-schema/double-type.json: In struct 'bar':
 tests/qapi-schema/double-type.json:2: unknown key 'command' in struct 'bar'
 Valid keys are 'base', 'data', 'features', 'if', 'struct'.
diff --git a/tests/qapi-schema/enum-missing-data.err b/tests/qapi-schema/enum-missing-data.err
index 3c3c52d037..ffde1082c3 100644
--- a/tests/qapi-schema/enum-missing-data.err
+++ b/tests/qapi-schema/enum-missing-data.err
@@ -1 +1,2 @@ 
+tests/qapi-schema/enum-missing-data.json: In enum 'MyEnum':
 tests/qapi-schema/enum-missing-data.json:2: key 'data' is missing from enum 'MyEnum'
diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
index 07558edb78..e401efe148 100644
--- a/tests/qapi-schema/unknown-expr-key.err
+++ b/tests/qapi-schema/unknown-expr-key.err
@@ -1,2 +1,3 @@ 
+tests/qapi-schema/unknown-expr-key.json: In struct 'bar':
 tests/qapi-schema/unknown-expr-key.json:2: unknown keys 'bogus', 'phony' in struct 'bar'
 Valid keys are 'base', 'data', 'features', 'if', 'struct'.