| Message ID | 1383611860-9053-4-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 11/04/2013 05:37 PM, Wenchao Xia wrote: > It will check whether the values specfied are wrotten correctly when s/specfied/specified/ s/wrotten/written/ > discriminator is a pre-defined enum type, which help check whether the > schema is in good form. > > It is allowed that, not every value in enum is used, so do not check s/that,/that/ > that case. Why do you allow partial coverage? That feels like an accident waiting to happen. Does the user get a sane error message if they request an enum value that wasn't mapped to a union branch? I think it would be wiser to mandate that if the discriminator is an enum, then the union must cover all values of the enum. > + > +# Return the descriminator enum define, if discriminator is specified in s/descriminator/discriminator/ > +# @expr and it is a pre-defined enum type > +def descriminator_find_enum_define(expr): s/descriminator/discriminator/ - and fix all callers > + discriminator = expr.get('discriminator') > + base = expr.get('base') > + > + # Only support discriminator when base present > + if not (discriminator and base): > + return None > + > + base_fields = find_base_fields(base) > + > + if not base_fields: > + sys.stderr.write("Base '%s' is not a valid type\n" > + % base) > + sys.exit(1) > + > + descriminator_type = base_fields.get(discriminator) s/descriminator/discriminator/
δΊ 2013/11/5 21:25, Eric Blake ει: > On 11/04/2013 05:37 PM, Wenchao Xia wrote: >> It will check whether the values specfied are wrotten correctly when > > s/specfied/specified/ > s/wrotten/written/ > >> discriminator is a pre-defined enum type, which help check whether the >> schema is in good form. >> >> It is allowed that, not every value in enum is used, so do not check > > s/that,/that/ > >> that case. > > Why do you allow partial coverage? That feels like an accident waiting > to happen. Does the user get a sane error message if they request an > enum value that wasn't mapped to a union branch? I think it would be > wiser to mandate that if the discriminator is an enum, then the union > must cover all values of the enum. > abort() will be called in qapi-visit.c, it is OK for output visitor, but bad for input visitor since user input may trigger it. I think change abort() to error_set() could solve the problem, then we allow map part of enum value. >> + >> +# Return the descriminator enum define, if discriminator is specified in > > s/descriminator/discriminator/ > >> +# @expr and it is a pre-defined enum type >> +def descriminator_find_enum_define(expr): > > s/descriminator/discriminator/ - and fix all callers > >> + discriminator = expr.get('discriminator') >> + base = expr.get('base') >> + >> + # Only support discriminator when base present >> + if not (discriminator and base): >> + return None >> + >> + base_fields = find_base_fields(base) >> + >> + if not base_fields: >> + sys.stderr.write("Base '%s' is not a valid type\n" >> + % base) >> + sys.exit(1) >> + >> + descriminator_type = base_fields.get(discriminator) > > s/descriminator/discriminator/ >
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index c39e628..803d3eb 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -249,6 +249,17 @@ def generate_visit_union(expr): assert not base return generate_visit_anon_union(name, members) + # If discriminator is specified and it is a pre-defined enum in schema, + # check its correctness + enum_define = descriminator_find_enum_define(expr) + if enum_define: + for key in members: + if not key in enum_define["enum_values"]: + sys.stderr.write("Discriminator value '%s' not found in " + "enum '%s'\n" % + (key, enum_define["enum_name"])) + sys.exit(1) + ret = generate_visit_enum('%sKind' % name, members.keys()) if base: diff --git a/scripts/qapi.py b/scripts/qapi.py index 82f586e..235df4f 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -383,3 +383,36 @@ def guardend(name): ''', name=guardname(name)) + +# This function can be used to check whether "base" is valid +def find_base_fields(base): + base_struct_define = find_struct(base) + if not base_struct_define: + return None + return base_struct_define.get('data') + +# Return the descriminator enum define, if discriminator is specified in +# @expr and it is a pre-defined enum type +def descriminator_find_enum_define(expr): + discriminator = expr.get('discriminator') + base = expr.get('base') + + # Only support discriminator when base present + if not (discriminator and base): + return None + + base_fields = find_base_fields(base) + + if not base_fields: + sys.stderr.write("Base '%s' is not a valid type\n" + % base) + sys.exit(1) + + descriminator_type = base_fields.get(discriminator) + + if not descriminator_type: + sys.stderr.write("Discriminator '%s' not found in schema\n" + % discriminator) + sys.exit(1) + + return find_enum(descriminator_type)
It will check whether the values specfied are wrotten correctly when discriminator is a pre-defined enum type, which help check whether the schema is in good form. It is allowed that, not every value in enum is used, so do not check that case. Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com> --- scripts/qapi-visit.py | 11 +++++++++++ scripts/qapi.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 0 deletions(-)