On 09/29/2014 02:38 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
