Eric Blake <ebl...@redhat.com> writes:
> Now that we have a way to validate every type, we can also be
> stricter about enforcing that callers that want to bypass
> type safety in generated code. Prior to this patch, it didn't
> matter what value was associated with the key 'gen', but it
> looked odd that 'gen':'yes' could result in bypassing the
> generated code. These changes also enforce the changes made
> earlier in the series for documentation and consolidation of
> using '**' as the wildcard type.
Perhaps it's worth mentioning that the schema has always used 'gen':
'no'.
That said, 'no' is silly. The natural JSON for a flag is false / true!
Since you're touching it anyway, consider making it an optional boolean
defaulting to false. Same for the other silly 'no': success-response.
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> scripts/qapi.py | 21 ++++++++++++++++-----
> tests/qapi-schema/type-bypass-bad-gen.err | 1 +
> tests/qapi-schema/type-bypass-bad-gen.exit | 2 +-
> tests/qapi-schema/type-bypass-bad-gen.json | 2 +-
> tests/qapi-schema/type-bypass-bad-gen.out | 3 ---
> tests/qapi-schema/type-bypass-no-gen.err | 1 +
> tests/qapi-schema/type-bypass-no-gen.exit | 2 +-
> tests/qapi-schema/type-bypass-no-gen.json | 2 +-
> tests/qapi-schema/type-bypass-no-gen.out | 3 ---
> 9 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 20c0ce9..15972c6 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -256,13 +256,13 @@ def discriminator_find_enum_define(expr):
> return find_enum(discriminator_type)
>
> def check_type(expr_info, source, data, allow_array = False,
> - allowed_names = [], allow_dict = True):
> + allowed_names = [], allow_dict = True, allow_star = False):
> global all_types
>
> if data is None:
> return
>
> - if data == '**':
> + if allow_star and data == '**':
> return
>
> # Check if array type for data is okay
> @@ -278,6 +278,10 @@ def check_type(expr_info, source, data, allow_array =
> False,
>
> # Check if type name for data is okay
> if isinstance(data, basestring):
> + if data == '**':
> + raise QAPIExprError(expr_info,
> + "%s uses '**' but did not request 'gen':'no'"
> + % source)
> if not data in all_types:
> raise QAPIExprError(expr_info,
> "%s references unknown type '%s'"
I'm blind: I can't see where this error gets suppressed when we have
'gen': 'no'.
> @@ -299,24 +303,27 @@ def check_type(expr_info, source, data, allow_array =
> False,
> check_type(expr_info, "member '%s' of %s" % (key, source), value,
> allow_array=True,
> allowed_names=['built-in', 'union', 'struct', 'enum'],
> - allow_dict=True)
> + allow_dict=True, allow_star=allow_star)
>
allow_star applies recursively. Correct, because...
> def check_command(expr, expr_info):
> global commands
> name = expr['command']
> + allow_star = expr.has_key('gen')
> +
> if name in commands:
> raise QAPIExprError(expr_info,
> "command '%s' is already defined" % name)
> commands.append(name)
> check_type(expr_info, "'data' for command '%s'" % name,
> expr.get('data'), allow_array=True,
> - allowed_names=['union', 'struct'])
> + allowed_names=['union', 'struct'], allow_star=allow_star)
> check_type(expr_info, "'base' for command '%s'" % name,
> expr.get('base'), allowed_names=['struct'],
> allow_dict=False)
> check_type(expr_info, "'returns' for command '%s'" % name,
> expr.get('returns'), allow_array=True,
> - allowed_names=['built-in', 'union', 'struct', 'enum'])
> + allowed_names=['built-in', 'union', 'struct', 'enum'],
> + allow_star=allow_star)
>
... it applies exactly to a command's 'data' and 'returns' when it has
'gen': 'no'. Good.
> def check_event(expr, expr_info):
> global events
> @@ -450,6 +457,10 @@ def check_keys(expr_elem, meta, required, optional=[]):
> raise QAPIExprError(info,
> "%s '%s' has unknown key '%s'"
> % (meta, name, key))
> + if (key == 'gen' or key == 'success-response') and value != 'no':
> + raise QAPIExprError(info,
> + "'%s' of %s '%s' should only have value 'no'"
> + % (key, meta, name))
> for key in required:
> if not expr.has_key(key):
> raise QAPIExprError(info,
[...]
