Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> ---
>  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,
[...]