| Message ID | 1411165504-18198-15-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@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 <eblake@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, [...]
On 09/29/2014 02:38 AM, Markus Armbruster wrote: > Eric Blake <eblake@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. 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 >> @@ -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. > >> @@ -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') >> + [2] The other trick to note here is that this only checks if 'gen' is a key; but at [3], which is run earlier, we further required that 'gen' can only appear if it has value 'no'. >> 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)) [3] >> for key in required: >> if not expr.has_key(key): >> raise QAPIExprError(info, > [...] > >
Eric Blake <eblake@redhat.com> writes: > On 09/29/2014 02:38 AM, Markus Armbruster wrote: >> Eric Blake <eblake@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. > > 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! [...]
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'" @@ -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) 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) 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, diff --git a/tests/qapi-schema/type-bypass-bad-gen.err b/tests/qapi-schema/type-bypass-bad-gen.err index e69de29..c408364 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.err +++ b/tests/qapi-schema/type-bypass-bad-gen.err @@ -0,0 +1 @@ +tests/qapi-schema/type-bypass-bad-gen.json:2: 'gen' of command 'foo' should only have value 'no' diff --git a/tests/qapi-schema/type-bypass-bad-gen.exit b/tests/qapi-schema/type-bypass-bad-gen.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.exit +++ b/tests/qapi-schema/type-bypass-bad-gen.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/type-bypass-bad-gen.json b/tests/qapi-schema/type-bypass-bad-gen.json index 6894526..5514b80 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.json +++ b/tests/qapi-schema/type-bypass-bad-gen.json @@ -1,2 +1,2 @@ -# FIXME: 'gen' should only appear with value 'no' +# 'gen' should only appear with value 'no' { 'command': 'foo', 'gen': 'whatever' } diff --git a/tests/qapi-schema/type-bypass-bad-gen.out b/tests/qapi-schema/type-bypass-bad-gen.out index e678f2c..e69de29 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.out +++ b/tests/qapi-schema/type-bypass-bad-gen.out @@ -1,3 +0,0 @@ -[OrderedDict([('command', 'foo'), ('gen', 'whatever')])] -[] -[] diff --git a/tests/qapi-schema/type-bypass-no-gen.err b/tests/qapi-schema/type-bypass-no-gen.err index e69de29..294ca2e 100644 --- a/tests/qapi-schema/type-bypass-no-gen.err +++ b/tests/qapi-schema/type-bypass-no-gen.err @@ -0,0 +1 @@ +tests/qapi-schema/type-bypass-no-gen.json:2: member 'arg' of 'data' for command 'unsafe' uses '**' but did not request 'gen':'no' diff --git a/tests/qapi-schema/type-bypass-no-gen.exit b/tests/qapi-schema/type-bypass-no-gen.exit index 573541a..d00491f 100644 --- a/tests/qapi-schema/type-bypass-no-gen.exit +++ b/tests/qapi-schema/type-bypass-no-gen.exit @@ -1 +1 @@ -0 +1 diff --git a/tests/qapi-schema/type-bypass-no-gen.json b/tests/qapi-schema/type-bypass-no-gen.json index 72c817f..49b5742 100644 --- a/tests/qapi-schema/type-bypass-no-gen.json +++ b/tests/qapi-schema/type-bypass-no-gen.json @@ -1,2 +1,2 @@ -# FIXME: type bypass should only work with 'gen':'no' +# type bypass only works with 'gen':'no' { 'command': 'unsafe', 'data': { 'arg': '**' }, 'returns': '**' } diff --git a/tests/qapi-schema/type-bypass-no-gen.out b/tests/qapi-schema/type-bypass-no-gen.out index 8b2a9ac..e69de29 100644 --- a/tests/qapi-schema/type-bypass-no-gen.out +++ b/tests/qapi-schema/type-bypass-no-gen.out @@ -1,3 +0,0 @@ -[OrderedDict([('command', 'unsafe'), ('data', OrderedDict([('arg', '**')])), ('returns', '**')])] -[] -[]
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. Signed-off-by: Eric Blake <eblake@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(-)