[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass |
Date: |
Mon, 29 Sep 2014 18:35:29 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 09/29/2014 02:38 AM, Markus Armbruster wrote:
>> 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.
>
> I'd love to, except that the .json parser does not yet allow literal
> true/false JSON keywords. Fam had a patch back in May that would fix
> that; maybe I'll fold in his patch to my v5 as a prereq patch:
>
> https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg03916.html
Your choice. I certainly don't want the silly 'no' block your series.
>>> @@ -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
>
> [1]
>
>>>
>>> # 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'.
>
> 'gen':'no' triggers the caller to pass allow_star=True to check_type
> [2]; then at point [1] we short-circuit and exit if '**' was passed.
> Therefore, if we get here and still have '**', then allow_star is still
> false, which means 'gen':'no' was not passed and it is user error.
Got it, thanks!
[...]
- Re: [Qemu-devel] [PATCH v4 19/19] qapi: Drop support for inline subtypes, (continued)
- [Qemu-devel] [PATCH v4 06/19] qapi: Better error messages for bad enums, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 18/19] qapi: Drop inline subtype in query-pci, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 15/19] qapi: Merge UserDefTwo and UserDefNested in tests, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 14/19] qapi: More rigorous checking for type safety bypass, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 17/19] qapi: Drop inline subtype in query-version, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 01/19] qapi: Consistent whitespace in tests/Makefile, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 16/19] qapi: Drop tests for inline subtypes, Eric Blake, 2014/09/19
- [Qemu-devel] [PATCH v4 02/19] qapi: Ignore files created during make check, Eric Blake, 2014/09/19
- Re: [Qemu-devel] [PATCH v4 00/19] drop qapi nested structs, Eric Blake, 2014/09/26