Re: [Qemu-devel] [PATCH for-2.9 05/47] qapi: Have each QAPI schema declare its returns white-list

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3270 bytes --]

On 03/13/2017 01:18 AM, Markus Armbruster wrote:
> qapi.py has a hardcoded white-list of command names that may violate
> the rules on permitted return types.  Add a new pragma directive
> 'returns-whitelist', and use it to replace the hard-coded white-list.

So now the list is per-client, rather than global. Nice idea!

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/qapi-schema.json
> @@ -51,6 +51,18 @@
>  
>  { 'pragma': { 'doc-required': true } }
>  
> +# Whitelists to permit QAPI rule violations; think twice before you
> +# add to them!
> +{ 'pragma': {
> +    # Commands allowed to return a non-dictionary:
> +    'returns-whitelist': [
> +        'human-monitor-command',
> +        'qom-get',
> +        'query-migrate-cache-size',
> +        'query-tpm-models',
> +        'query-tpm-types',
> +        'ringbuf-read' ] } }
> +

If I'm understanding the code right, we could have also written this all
as one pragma with a larger dict instead of two pragmas with one-element
dicts:

{ 'pragma': { 'doc-required': true,
              'returns-whitelist': [ ... ] } }

But see below about another potential for rewriting that I thought of
before reading your full patch [1]...

> @@ -317,12 +298,19 @@ class QAPISchemaParser(object):
>          self.docs.extend(exprs_include.docs)
>  
>      def _pragma(self, name, value, info):
> -        global doc_required
> +        global doc_required, returns_whitelist
>          if name == 'doc-required':
>              if not isinstance(value, bool):
>                  raise QAPISemError(info,
>                                     "Pragma 'doc-required' must be boolean")
>              doc_required = value
> +        elif name == 'returns-whitelist':
> +            if (not isinstance(value, list)
> +                    or any([not isinstance(elt, str) for elt in value])):
> +                raise QAPISemError(info,
> +                                   "Pragma returns-whitelist must be"
> +                                   " a list of strings")

Again, a new error message with no testsuite coverage.

> +            returns_whitelist = value

[1] Hmm, this precludes the converse direction of specifying things.
You cannot usefully list the whitelist pragma more than once, because
only the last one wins.  Why would we want to allow it to be more than
once? because we could do:

{ 'pragma': 'returns-whitelist': [ 'human-monitor-command' ] }
{ 'pragma': 'returns-whitelist': [ 'qom-get' ] }

and then spread out the uses of the pragma to be closer to the
violations, rather than bunched up front.

Or maybe you want to consider rejecting a second whitelist, instead of
silently losing the first one, if you want to force that all violations
are bunched up front into a single pragma.

But that's food for thought - I'm leaving it up to you if you want to
spin a v2 (making non-trivial changes based on my comments), or leave
improvements (like any testsuite additions) for a followup patch.  If
you use this patch as-is, you can add:

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]