[v11,03/15] qapi: Forbid 'any' inside an alternate

Commit Message

Eric Blake Feb. 18, 2016, 8:11 p.m. UTC

On 02/18/2016 10:03 AM, Markus Armbruster wrote:

>>> Could use a test for alternate member of alternate type.
>>
>> One step ahead of you: commit 3d0c4829 added the test
>> alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
>> parser to reject it (first by a hard-coded check, then via allow_metas[]
>> excluding alternates).  'any' is the only value that could sneak
>> through, because it is a subset of 'built-in' which allow_metas[]
>> whitelisted.
> 
> Then find_alternate_member_qtype()'s final return None is unreachable,
> correct?

Indeed, the testsuite still passes with:



That said, even though we currently filter out unknown types before
deciding to call find_alternate_member_qtype, it's not out of the
question that future work to move ad hoc front-end tests into formal
QAPISchema .check() methods may cause us to call
find_alternate_member_qtype('unknown').  Leaving it as return None
instead of asserting would make the error message added in this patch
nicer.  Then again, refactoring would move the error message of this
patch to the .check() methods.  So I won't worry about it for now.

Comments

Markus Armbruster Feb. 19, 2016, 8:32 a.m. UTC | #1

Eric Blake <[e-mail]> writes:

> On 02/18/2016 10:03 AM, Markus Armbruster wrote:
>
>>>> Could use a test for alternate member of alternate type.
>>>
>>> One step ahead of you: commit 3d0c4829 added the test
>>> alternate-nested.json, and commits 44bd1276 and dd883c6f fixed the
>>> parser to reject it (first by a hard-coded check, then via allow_metas[]
>>> excluding alternates).  'any' is the only value that could sneak
>>> through, because it is a subset of 'built-in' which allow_metas[]
>>> whitelisted.
>> 
>> Then find_alternate_member_qtype()'s final return None is unreachable,
>> correct?
>
> Indeed, the testsuite still passes with:
>
> diff --git i/scripts/qapi.py w/scripts/qapi.py
> index 8497777..81d435f 100644
> --- i/scripts/qapi.py
> +++ w/scripts/qapi.py
> @@ -345,7 +345,7 @@ def find_alternate_member_qtype(qapi_type):
>          return "QTYPE_QSTRING"
>      elif find_union(qapi_type):
>          return "QTYPE_QDICT"
> -    return None
> +    assert False
>
>
>  # Return the discriminator enum define if discriminator is specified as an
>
>
> That said, even though we currently filter out unknown types before
> deciding to call find_alternate_member_qtype, it's not out of the
> question that future work to move ad hoc front-end tests into formal
> QAPISchema .check() methods may cause us to call
> find_alternate_member_qtype('unknown').  Leaving it as return None
> instead of asserting would make the error message added in this patch
> nicer.  Then again, refactoring would move the error message of this
> patch to the .check() methods.  So I won't worry about it for now.

Makes sense.  I was just making sure I understand how this works.
Thanks!

Patch

diff --git i/scripts/qapi.py w/scripts/qapi.py
index 8497777..81d435f 100644
--- i/scripts/qapi.py
+++ w/scripts/qapi.py
@@ -345,7 +345,7 @@  def find_alternate_member_qtype(qapi_type):
         return "QTYPE_QSTRING"
     elif find_union(qapi_type):
         return "QTYPE_QDICT"
-    return None
+    assert False


 # Return the discriminator enum define if discriminator is specified as an