| Message ID | 20190624173935.25747-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Try to create well-typed json:{} filenames | expand |
Less than thorough review, because I expect the necessary rebase will require a bit of rewriting here and there. Max Reitz <mreitz@redhat.com> writes: > With this change, it is possible to give default values for struct > members, as follows: > > What you had to do so far: > > # @member: Some description, defaults to 42. > { 'struct': 'Foo', > 'data': { '*member': 'int' } } > > What you can do now: > > { 'struct': 'Foo', > 'data': { '*member': { 'type': 'int', 'default': 42 } } > > On the C side, this change would remove Foo.has_member, because > Foo.member is always valid now. The input visitor deals with setting > it. (Naturally, this means that such defaults are useful only for input > parameters.) > > At least three things are left unimplemented: > > First, support for alternate data types. This is because supporting > them would mean having to allocate the object in the input visitor, and > then potentially going through multiple levels of nested types. In any > case, it would have been difficult and I do not think there is need for > such support at this point. I don't mind restricting the 'default' feature to uses we actually have, at least initially. I'm afraid I don't fully understand the difficulties you describe, but I guess that's okay. > Second, support for null. The most important reason for this is that > introspection already uses "'default': null" for "no default, but this > field is optional". The second reason is that without support for > alternate data types, there is not really a point in supporting null. Also, commit 9d55380b5a "qapi: Remove null from schema language" :) > Third, full support for default lists. This has a similar reason to the > lack of support for alternate data types: Allocating a default list is > not trivial -- unless the list is empty, which is exactly what we have > support for. Your commit message says "for struct members". What about union members? Cases: * Flat union 'base' members: 'base' is a a struct, possibly implicit. Do defaults work in implicit bases, like BlockdevOption's? * Flat union branch members: these are always struct types, so there's nothing for me to ask. I think. * Simple union branch members: these are each wrapped in an implicit struct type. Do defaults work? I'd be totally fine with "nope, not implemented, not going to implement it" here. > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qapi/introspect.json | 9 +- > scripts/qapi/commands.py | 2 +- > scripts/qapi/common.py | 167 +++++++++++++++++++++++++++++++++++-- > scripts/qapi/doc.py | 20 ++++- > scripts/qapi/introspect.py | 2 +- > scripts/qapi/types.py | 2 +- > scripts/qapi/visit.py | 38 ++++++++- > 7 files changed, 217 insertions(+), 23 deletions(-) Missing: docs/devel/qapi-code-gen.txt update. > > diff --git a/qapi/introspect.json b/qapi/introspect.json > index 1843c1cb17..db703135f9 100644 > --- a/qapi/introspect.json > +++ b/qapi/introspect.json > @@ -198,11 +198,10 @@ > # > # @default: default when used as command parameter. > # If absent, the parameter is mandatory. > -# If present, the value must be null. The parameter is > -# optional, and behavior when it's missing is not specified > -# here. > -# Future extension: if present and non-null, the parameter > -# is optional, and defaults to this value. > +# If present and null, the parameter is optional, and > +# behavior when it's missing is not specified here. > +# If present and non-null, the parameter is optional, and > +# defaults to this value. > # > # Since: 2.5 > ## > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > index b929e07be4..6c407cd4ba 100644 > --- a/scripts/qapi/commands.py > +++ b/scripts/qapi/commands.py > @@ -35,7 +35,7 @@ def gen_call(name, arg_type, boxed, ret_type): > elif arg_type: > assert not arg_type.variants > for memb in arg_type.members: > - if memb.optional: > + if memb.optional and memb.default is None: > argstr += 'arg.has_%s, ' % c_name(memb.name) > argstr += 'arg.%s, ' % c_name(memb.name) > > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index c6754a5856..8c57d0c67a 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -14,6 +14,7 @@ > from __future__ import print_function > from contextlib import contextmanager > import errno > +import math > import os > import re > import string > @@ -800,6 +801,136 @@ def check_if(expr, info): > check_if_str(ifcond, info) > > > +def check_value_str(info, value): > + return 'g_strdup(%s)' % to_c_string(value) if type(value) is str else False What's wrong with isinstance(value, str)? I'm a happy user of ternaries myself, but this one results in a rather long line. Easier to read, I think: if isinstance(value, str): return 'g_strdup(%s)' % to_c_string(value) return False > + > +def check_value_number(info, value): > + if type(value) is not float: > + return False > + if math.isinf(value): > + return 'INFINITY' if value > 0 else '-INFINITY' > + elif math.isnan(value): > + return 'NAN' > + else: > + return '%.16e' % value Why not str(value)? > + > +def check_value_bool(info, value): > + if type(value) is not bool: > + return False > + return 'true' if value else 'false' > + > +def is_int_type(value): > + if type(value) is int: > + return True > + # 'long' does not exist in Python 3 > + try: > + if type(value) is long: > + return True > + except NameError: > + pass > + > + return False Python 2 busy-work... > + > +def gen_check_value_int(bits): > + def check_value_int(info, value): > + if not is_int_type(value) or \ > + value < -(2 ** (bits - 1)) or value >= 2 ** (bits - 1): > + return False > + if bits > 32: > + return '%ill' % value > + else: > + return '%i' % value Why not str(value) regardless of @bits? > + > + return check_value_int > + > +def gen_check_value_uint(bits): > + def check_value_uint(info, value): > + if not is_int_type(value) or value < 0 or value >= 2 ** bits: > + return False > + if bits > 32: > + return '%uull' % value > + elif bits > 16: > + return '%uu' % value > + else: > + return '%u' % value Likewise. > + > + return check_value_uint Your check_value_FOO(info, value) have a peculiar contract: If @value is a valid FOO, convert it to str and return that. Else return False. @info is unused. I wouldn't guess that from the name. What about this: rename to str_if_FOO(), return @value converted to str if it's a valid FOO, else None. Ditch @info unless there's a reason to keep it. > + > +# Check whether the given value fits the given QAPI type. > +# If so, return a C representation of the value (pointers point to > +# newly allocated objects). > +# Otherwise, raise an exception. The parenthesis gave me pause (I figure my body's busy digesting lunch). The only pointer-valued type is 'str', isn't it? Type-checking is in schema.py now. Stuff like @enum_types is gone. See commit fa110c6a9e "qapi: Move context-sensitive checking to the proper place". > +def check_value(info, qapi_type, value): > + builtin_type_checks = { > + 'str': check_value_str, > + 'int': gen_check_value_int(64), > + 'number': check_value_number, > + 'bool': check_value_bool, > + 'int8': gen_check_value_int(8), > + 'int16': gen_check_value_int(16), > + 'int32': gen_check_value_int(32), > + 'int64': gen_check_value_int(64), > + 'uint8': gen_check_value_uint(8), > + 'uint16': gen_check_value_uint(16), > + 'uint32': gen_check_value_uint(32), > + 'uint64': gen_check_value_uint(64), > + 'size': gen_check_value_uint(64), > + } > + > + # Cannot support null because that would require a value of "None" > + # (which is reserved for no default) This is another instance of the class of problems that led to commit 9d55380b5a "qapi: Remove null from schema language". > + unsupported_builtin_types = ['null', 'any', 'QType'] You give a clue for 'null'. Should you give one for 'any' and 'QType', too? > + > + if type(qapi_type) is list: > + has_list = True > + qapi_type = qapi_type[0] > + elif qapi_type.endswith('List'): > + has_list = True > + qapi_type = qapi_type[:-4] > + else: > + has_list = False > + > + if has_list: > + if value == []: > + return 'NULL' > + else: > + raise QAPISemError(info, > + "Support for non-empty lists as default values has not been " \ > + "implemented yet: '{}'".format(value)) > + > + if qapi_type in builtin_type_checks: > + c_val = builtin_type_checks[qapi_type](info, value) > + if not c_val: > + raise QAPISemError(info, > + "Value '{}' does not match type {}".format(value, qapi_type)) > + return c_val > + > + if qapi_type in unsupported_builtin_types: > + raise QAPISemError(info, > + "Cannot specify values for type %s" % qapi_type) > + > + if qapi_type in enum_types: > + if not check_value_str(info, value): > + raise QAPISemError(info, > + "Enum values must be strings, but '{}' is no string" \ > + .format(value)) > + > + enum_values = enum_types[qapi_type]['data'] > + for ev in enum_values: > + if ev['name'] == value: > + return c_enum_const(qapi_type, value, > + enum_types[qapi_type].get('prefix')) > + > + raise QAPISemError(info, > + "Value '{}' does not occur in enum {}".format(value, qapi_type)) > + > + # TODO: Support alternates > + > + raise QAPISemError(info, > + "Cannot specify values for type %s (not built-in or an enum)" % > + qapi_type) > + > + check_value() furses checking and converting. Its callers seem to need either the checking or the converting. I'm not sure fusing the two is a good idea. > def check_type(info, source, value, allow_array=False, > allow_dict=False, allow_optional=False, > allow_metas=[]): > @@ -842,15 +973,22 @@ def check_type(info, source, value, allow_array=False, > if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): > raise QAPISemError(info, "Member of %s uses reserved name '%s'" > % (source, key)) > - # Todo: allow dictionaries to represent default values of > - # an optional argument. > + > check_known_keys(info, "member '%s' of %s" % (key, source), > - arg, ['type'], ['if']) > + arg, ['type'], ['if', 'default']) > check_type(info, "Member '%s' of %s" % (key, source), > arg['type'], allow_array=True, > allow_metas=['built-in', 'union', 'alternate', 'struct', > 'enum']) > > + if 'default' in arg: > + if key[0] != '*': > + raise QAPISemError(info, > + "'%s' is not optional, so it cannot have a default value" % > + key) > + > + check_value(info, arg['type'], arg['default']) > + > > def check_command(expr, info): > name = expr['command'] > @@ -1601,13 +1739,14 @@ class QAPISchemaFeature(QAPISchemaMember): > > > class QAPISchemaObjectTypeMember(QAPISchemaMember): > - def __init__(self, name, typ, optional, ifcond=None): > + def __init__(self, name, typ, optional, ifcond=None, default=None): > QAPISchemaMember.__init__(self, name, ifcond) > assert isinstance(typ, str) > assert isinstance(optional, bool) > self._type_name = typ > self.type = None > self.optional = optional > + self.default = default > > def check(self, schema): > assert self.owner > @@ -1917,7 +2056,7 @@ class QAPISchema(object): > name, info, doc, ifcond, > self._make_enum_members(data), prefix)) > > - def _make_member(self, name, typ, ifcond, info): > + def _make_member(self, name, typ, ifcond, default, info): > optional = False > if name.startswith('*'): > name = name[1:] > @@ -1925,10 +2064,11 @@ class QAPISchema(object): > if isinstance(typ, list): > assert len(typ) == 1 > typ = self._make_array_type(typ[0], info) > - return QAPISchemaObjectTypeMember(name, typ, optional, ifcond) > + return QAPISchemaObjectTypeMember(name, typ, optional, ifcond, default) > > def _make_members(self, data, info): > - return [self._make_member(key, value['type'], value.get('if'), info) > + return [self._make_member(key, value['type'], value.get('if'), > + value.get('default'), info) > for (key, value) in data.items()] > > def _def_struct_type(self, expr, info, doc): > @@ -1951,7 +2091,7 @@ class QAPISchema(object): > typ = self._make_array_type(typ[0], info) > typ = self._make_implicit_object_type( > typ, info, None, self.lookup_type(typ), > - 'wrapper', [self._make_member('data', typ, None, info)]) > + 'wrapper', [self._make_member('data', typ, None, None, info)]) > return QAPISchemaObjectTypeVariant(case, typ, ifcond) > > def _def_union_type(self, expr, info, doc): > @@ -2234,6 +2374,15 @@ def to_c_string(string): > return result > > > +# Translates a value for the given QAPI type to its C representation. > +# The caller must have called check_value() during parsing to be sure > +# that the given value fits the type. > +def c_value(qapi_type, value): > + pseudo_info = {'file': '(generator bug)', 'line': 0, 'parent': None} > + # The caller guarantees this does not raise an exception > + return check_value(pseudo_info, qapi_type, value) > + > + > def guardstart(name): > return mcgen(''' > #ifndef %(name)s > @@ -2356,7 +2505,7 @@ def build_params(arg_type, boxed, extra=None): > for memb in arg_type.members: > ret += sep > sep = ', ' > - if memb.optional: > + if memb.optional and memb.default is None: > ret += 'bool has_%s, ' % c_name(memb.name) > ret += '%s %s' % (memb.type.c_param_type(), > c_name(memb.name)) > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py > index 5fc0fc7e06..78a9052738 100755 > --- a/scripts/qapi/doc.py > +++ b/scripts/qapi/doc.py > @@ -139,13 +139,29 @@ def texi_enum_value(value, desc, suffix): > value.name, desc, texi_if(value.ifcond, prefix='@*')) > > > +def doc_value(value): > + if value is True: > + return 'true' > + elif value is False: > + return 'false' > + elif value is None: > + return 'null' > + else: > + return '{}'.format(value) > + > def texi_member(member, desc, suffix): > """Format a table of members item for an object type member""" > typ = member.type.doc_type() > membertype = ': ' + typ if typ else '' > + > + optional_info = '' > + if member.default is not None: > + optional_info = ' (optional, default: %s)' % doc_value(member.default) > + elif member.optional: > + optional_info = ' (optional)' > + > return '@item @code{%s%s}%s%s\n%s%s' % ( > - member.name, membertype, > - ' (optional)' if member.optional else '', > + member.name, membertype, optional_info, > suffix, desc, texi_if(member.ifcond, prefix='@*')) > > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 572e0b8331..7d73020a42 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -159,7 +159,7 @@ const QLitObject %(c_name)s = %(c_string)s; > def _gen_member(self, member): > ret = {'name': member.name, 'type': self._use_type(member.type)} > if member.optional: > - ret['default'] = None > + ret['default'] = member.default > if member.ifcond: > ret = (ret, {'if': member.ifcond}) > return ret > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 3edd9374aa..46a6d33379 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -44,7 +44,7 @@ def gen_struct_members(members): > ret = '' > for memb in members: > ret += gen_if(memb.ifcond) > - if memb.optional: > + if memb.optional and memb.default is None: > ret += mcgen(''' > bool has_%(c_name)s; > ''', > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 484ebb66ad..0960e25a25 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -40,10 +40,14 @@ def gen_visit_object_members(name, base, members, variants): > void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > { > Error *err = NULL; > - > ''', > c_name=c_name(name)) > > + if len([m for m in members if m.default is not None]) > 0: > + ret += mcgen(''' > + bool has_optional; > +''') > + > if base: > ret += mcgen(''' > visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err); > @@ -53,13 +57,28 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > ''', > c_type=base.c_name()) > > + ret += mcgen(''' > + > +''') > + > for memb in members: > ret += gen_if(memb.ifcond) > if memb.optional: > + if memb.default is not None: > + optional_target = 'has_optional' > + # Visitors other than the input visitor do not have to implement > + # .optional(). Therefore, we have to initialize has_optional. Suggest "Only input visitors must implement .optional()." > + # Initialize it to true, because the field's value is always > + # present when using any visitor but the input visitor. > + ret += mcgen(''' > + has_optional = true; > +''') > + else: > + optional_target = 'obj->has_' + c_name(memb.name) > ret += mcgen(''' > - if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) { > + if (visit_optional(v, "%(name)s", &%(opt_target)s)) { > ''', > - name=memb.name, c_name=c_name(memb.name)) > + name=memb.name, opt_target=optional_target) > push_indent() > ret += mcgen(''' > visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, &err); I've stared at this dumbly for too long. It can't actually be that hard. I'm afraid I've run out of steam for today. I'll continue when my steam pressure is back to operational. > @@ -69,7 +88,16 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > ''', > c_type=memb.type.c_name(), name=memb.name, > c_name=c_name(memb.name)) > - if memb.optional: > + if memb.default is not None: > + pop_indent() > + ret += mcgen(''' > + } else { > + obj->%(c_name)s = %(c_value)s; > + } > +''', > + c_name=c_name(memb.name), > + c_value=c_value(memb._type_name, memb.default)) > + elif memb.optional: > pop_indent() > ret += mcgen(''' > } > @@ -287,6 +315,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > self._add_system_module(None, ' * Built-in QAPI visitors') > self._genc.preamble_add(mcgen(''' > #include "qemu/osdep.h" > +#include <math.h> > #include "qapi/error.h" > #include "qapi/qapi-builtin-visit.h" > ''')) > @@ -302,6 +331,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > visit = self._module_basename('qapi-visit', name) > self._genc.preamble_add(mcgen(''' > #include "qemu/osdep.h" > +#include <math.h> > #include "qapi/error.h" > #include "qapi/qmp/qerror.h" > #include "%(visit)s.h"
Max Reitz <mreitz@redhat.com> writes: > With this change, it is possible to give default values for struct > members, as follows: > > What you had to do so far: > > # @member: Some description, defaults to 42. > { 'struct': 'Foo', > 'data': { '*member': 'int' } } > > What you can do now: > > { 'struct': 'Foo', > 'data': { '*member': { 'type': 'int', 'default': 42 } } The '*' is redundant in this form. Can anyone think of reasons for keeping it anyway? Against? > On the C side, this change would remove Foo.has_member, because > Foo.member is always valid now. The input visitor deals with setting > it. (Naturally, this means that such defaults are useful only for input > parameters.) > > At least three things are left unimplemented: > > First, support for alternate data types. This is because supporting > them would mean having to allocate the object in the input visitor, and > then potentially going through multiple levels of nested types. In any > case, it would have been difficult and I do not think there is need for > such support at this point. > > Second, support for null. The most important reason for this is that > introspection already uses "'default': null" for "no default, but this > field is optional". The second reason is that without support for > alternate data types, there is not really a point in supporting null. > > Third, full support for default lists. This has a similar reason to the > lack of support for alternate data types: Allocating a default list is > not trivial -- unless the list is empty, which is exactly what we have > support for. > > Signed-off-by: Max Reitz <mreitz@redhat.com>
On 11/21/19 9:07 AM, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> With this change, it is possible to give default values for struct >> members, as follows: >> >> What you had to do so far: >> >> # @member: Some description, defaults to 42. >> { 'struct': 'Foo', >> 'data': { '*member': 'int' } } >> >> What you can do now: >> >> { 'struct': 'Foo', >> 'data': { '*member': { 'type': 'int', 'default': 42 } } > > The '*' is redundant in this form. > > Can anyone think of reasons for keeping it anyway? Against? Is there ever a reason to allow an optional member but without a 'default' value? Or can we just blindly state that if 'default' is not present, that is the same as 'default':0/'default':null? Or, applying your statement further, 'data': { '*a':'int', '*b':'str' } is shorthand for: 'data': { 'a': { 'type':'int', 'default':0 }, 'b': { 'type':'str', 'default':null } } So I could live with permitting '*' only in the shorthand form, and declaring that it is incompatible with longhand form because the existence of a 'default' key in longhand form is evidence that the member is therefore optional.
Eric Blake <eblake@redhat.com> writes: > On 11/21/19 9:07 AM, Markus Armbruster wrote: >> Max Reitz <mreitz@redhat.com> writes: >> >>> With this change, it is possible to give default values for struct >>> members, as follows: >>> >>> What you had to do so far: >>> >>> # @member: Some description, defaults to 42. >>> { 'struct': 'Foo', >>> 'data': { '*member': 'int' } } >>> >>> What you can do now: >>> >>> { 'struct': 'Foo', >>> 'data': { '*member': { 'type': 'int', 'default': 42 } } >> >> The '*' is redundant in this form. >> >> Can anyone think of reasons for keeping it anyway? Against? > > Is there ever a reason to allow an optional member but without a > 'default' value? Or can we just blindly state that if 'default' is > not present, that is the same as 'default':0/'default':null? > > Or, applying your statement further, > > 'data': { '*a':'int', '*b':'str' } > > is shorthand for: > > 'data': { 'a': { 'type':'int', 'default':0 }, > 'b': { 'type':'str', 'default':null } } You propose to default 'default' to a type-specific value. I don't think that's a good idea. Quoting myself on v3: In many programming languages, absent optional arguments / members default to a default value specified in the declaration. Simple. In others, 'absent' is effectively an additional value. The code consuming the argument / member can interpret 'absent' however it wants. It may eliminate the additional value by mapping it to a default value, but it can also interpret 'absent' unlike any value. If there's more than one consumer, their interpretations need not be consistent. The declaration provides no clue on semantics of 'absent'. QAPI is in the latter camp. I trust you can already sense how much I like that. [...] If I could go back in time, I'd flip QAPI to "'absent' defaults to a default value". Lacking a time machine, we're stuck with cases of "'absent' means something you can't express with a value" and "'absent' means different things in different contexts" that have been enshrined in external interfaces. Is there anything we could do to improve matters for saner cases? I think there is: we could provide for an *optional* default value. If the schema specifies it, that's what 'absent' means. If it doesn't, all bets are off, just like they are now. This patch implements this idea. When an absent member behaves just like it was present with a certain value DFLT, we want to be able to say in the schema 'default': DFLT. But the schema language also needs to let us say "absent member behaves unlike any value". If we define 'default' to default to a value, then that value must have this special meaning. Where that value is also a valid value, the schema language cannot express "absent member behaves like it was present with that value". I think this makes 0 a poor default value for 'default': "behaves like member was present with value 0" is fairly common, I think. Defaulting 'default' to null regardless of member type could work. null is a valid value of the 'null' type and of alternate types with a member of type 'null'. For optional members of such types, the schema language then can't express ""absent member behaves like it was present with value null". I think the need to say that is much less common than the needs to say "like value 0". Checking... *sigh*: there are a few optional members that can take null values, e.g. MigrateSetParameters member @tls-creds. I read its doc comment twice, and I have to admit I can't make heads or tails of it. Can't say for sure whether absent behaves like null, or some other value, or unlike any value. QAPI/QMP introspection already specifies null to have exactly this special meaning. > So I could live with permitting '*' only in the shorthand form, and > declaring that it is incompatible with longhand form because the > existence of a 'default' key in longhand form is evidence that the > member is therefore optional. Noted. More opinions?
On 11/21/19 1:46 PM, Markus Armbruster wrote: >>> The '*' is redundant in this form. >>> >>> Can anyone think of reasons for keeping it anyway? Against? >> >> Is there ever a reason to allow an optional member but without a >> 'default' value? Or can we just blindly state that if 'default' is >> not present, that is the same as 'default':0/'default':null? >> >> Or, applying your statement further, >> >> 'data': { '*a':'int', '*b':'str' } >> >> is shorthand for: >> >> 'data': { 'a': { 'type':'int', 'default':0 }, >> 'b': { 'type':'str', 'default':null } } > > You propose to default 'default' to a type-specific value. > > I don't think that's a good idea. ... > When an absent member behaves just like it was present with a certain > value DFLT, we want to be able to say in the schema 'default': DFLT. > > But the schema language also needs to let us say "absent member behaves > unlike any value". > > If we define 'default' to default to a value, then that value must have > this special meaning. > > Where that value is also a valid value, the schema language cannot > express "absent member behaves like it was present with that value". > > I think this makes 0 a poor default value for 'default': "behaves like > member was present with value 0" is fairly common, I think. > > Defaulting 'default' to null regardless of member type could work. > > null is a valid value of the 'null' type and of alternate types with a > member of type 'null'. For optional members of such types, the schema > language then can't express ""absent member behaves like it was present > with value null". I think the need to say that is much less common than > the needs to say "like value 0". > > Checking... *sigh*: there are a few optional members that can take null > values, e.g. MigrateSetParameters member @tls-creds. I read its doc > comment twice, and I have to admit I can't make heads or tails of it. > Can't say for sure whether absent behaves like null, or some other > value, or unlike any value. > > QAPI/QMP introspection already specifies null to have exactly this > special meaning. Maybe that means we need '*name':'t' to expand into something longer, maybe 'name':{'type':'t', 'optional':true} which in turn would be synonymous with your idea of ALL types accepting a default of null: 'name':{'type':'t', 'optional':true, 'default':null} At any rate, your counterpoint is taken - whatever we pick, we'll want to make sure that introspection can expose semantics, and whether we can make '*' redundant with some other form of longhand in the qapi language is in part determined by whether we also reflect that through introspection. If that means that keeping '*' in the longhand form of optional members (whether or not those members have a default value), then so be it.
Eric Blake <eblake@redhat.com> writes: > On 11/21/19 1:46 PM, Markus Armbruster wrote: > >>>> The '*' is redundant in this form. >>>> >>>> Can anyone think of reasons for keeping it anyway? Against? >>> >>> Is there ever a reason to allow an optional member but without a >>> 'default' value? Or can we just blindly state that if 'default' is >>> not present, that is the same as 'default':0/'default':null? >>> >>> Or, applying your statement further, >>> >>> 'data': { '*a':'int', '*b':'str' } >>> >>> is shorthand for: >>> >>> 'data': { 'a': { 'type':'int', 'default':0 }, >>> 'b': { 'type':'str', 'default':null } } >> >> You propose to default 'default' to a type-specific value. >> >> I don't think that's a good idea. > > ... > > >> When an absent member behaves just like it was present with a certain >> value DFLT, we want to be able to say in the schema 'default': DFLT. >> >> But the schema language also needs to let us say "absent member behaves >> unlike any value". >> >> If we define 'default' to default to a value, then that value must have >> this special meaning. >> >> Where that value is also a valid value, the schema language cannot >> express "absent member behaves like it was present with that value". >> >> I think this makes 0 a poor default value for 'default': "behaves like >> member was present with value 0" is fairly common, I think. >> >> Defaulting 'default' to null regardless of member type could work. >> >> null is a valid value of the 'null' type and of alternate types with a >> member of type 'null'. For optional members of such types, the schema >> language then can't express ""absent member behaves like it was present >> with value null". I think the need to say that is much less common than >> the needs to say "like value 0". >> >> Checking... *sigh*: there are a few optional members that can take null >> values, e.g. MigrateSetParameters member @tls-creds. I read its doc >> comment twice, and I have to admit I can't make heads or tails of it. >> Can't say for sure whether absent behaves like null, or some other >> value, or unlike any value. >> >> QAPI/QMP introspection already specifies null to have exactly this >> special meaning. > > Maybe that means we need '*name':'t' to expand into something longer, maybe > 'name':{'type':'t', 'optional':true} > which in turn would be synonymous with your idea of ALL types > accepting a default of null: > 'name':{'type':'t', 'optional':true, 'default':null} Yes, this is something we can consider. Currently, we normalize away the '*' prefix when we go from the abstract parse tree (which we call "expressions") to the internal representation: def _make_member(self, name, typ, ifcond, info): optional = False if name.startswith('*'): name = name[1:] optional = True if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) return QAPISchemaObjectTypeMember(name, info, typ, optional, ifcond) This keeps the normalization internal. Other normalizations we perform on the abstract parse tree. For instance, here's the one that normalizes shorthand member "'KEY': 'ARG'" to longhand "'KEY': { 'type': 'ARG' }": def normalize_members(members): if isinstance(members, OrderedDict): for key, arg in members.items(): if isinstance(arg, dict): continue members[key] = {'type': arg} For these normalizations, both the shorthand and the longhand form are part of the schema language. We could do the same for '*'. > At any rate, your counterpoint is taken - whatever we pick, we'll want > to make sure that introspection can expose semantics, and whether we > can make '*' redundant with some other form of longhand in the qapi > language is in part determined by whether we also reflect that through > introspection. Introspection has the true member name, without the '*' prefix. We'll also want to avoid unnecessary compromises on QAPI schema expressiveness. If we use null to mean "schema does not specify behavior when member is absent", we can't use it to mean "absent member behaves like the value null". A bit of a blemish, but I think it's a tolerable one. > If that means that keeping '*' in the longhand form of > optional members (whether or not those members have a default value), > then so be it. I believe both '*KEY': { 'type': ARG': 'default': null } and 'KEY': { 'type': ARG': 'default': null } are viable longhand forms for '*KEY': 'ARG'. I prefer the latter, but I'm open to arguments.
Am 22.11.2019 um 08:29 hat Markus Armbruster geschrieben: > > At any rate, your counterpoint is taken - whatever we pick, we'll want > > to make sure that introspection can expose semantics, and whether we > > can make '*' redundant with some other form of longhand in the qapi > > language is in part determined by whether we also reflect that through > > introspection. > > Introspection has the true member name, without the '*' prefix. > > We'll also want to avoid unnecessary compromises on QAPI schema > expressiveness. If we use null to mean "schema does not specify > behavior when member is absent", we can't use it to mean "absent member > behaves like the value null". A bit of a blemish, but I think it's a > tolerable one. If you want an example for an option that defaults to null, take the backing option of BlockdevOptionsGenericCOWFormat. What is the reason for even considering limiting the expressiveness? Do you think that an additional 'optional' bool, at least for those options that don't have a default, would be so bad in the longhand form? Or keeping '*' even in the longhand form, as suggested below. > > If that means that keeping '*' in the longhand form of > > optional members (whether or not those members have a default value), > > then so be it. > > I believe both > > '*KEY': { 'type': ARG': 'default': null } > > and > > 'KEY': { 'type': ARG': 'default': null } > > are viable longhand forms for '*KEY': 'ARG'. > > I prefer the latter, but I'm open to arguments. If you go for the former, then you certainly want to use absent 'default' to indicate no default, and allow a QNull default with 'default': null. The only reason to abuse 'default': null for no default is that you can't distinguish optional and non-optional if you use 'KEY' for both instead of 'KEY' for mandatory and '*KEY' for optional. So while I understand and to some degree share your dislike for the '*' prefix, I think I cast my pragmatic vote for: mandatory: 'KEY': { 'type': 'ARG' } optional without a default: '*KEY': { 'type': 'ARG' } optional with QNull default: '*KEY': { 'type': 'ARG', 'default': null } Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 22.11.2019 um 08:29 hat Markus Armbruster geschrieben: >> > At any rate, your counterpoint is taken - whatever we pick, we'll want >> > to make sure that introspection can expose semantics, and whether we >> > can make '*' redundant with some other form of longhand in the qapi >> > language is in part determined by whether we also reflect that through >> > introspection. >> >> Introspection has the true member name, without the '*' prefix. >> >> We'll also want to avoid unnecessary compromises on QAPI schema >> expressiveness. If we use null to mean "schema does not specify >> behavior when member is absent", we can't use it to mean "absent member >> behaves like the value null". A bit of a blemish, but I think it's a >> tolerable one. > > If you want an example for an option that defaults to null, take the > backing option of BlockdevOptionsGenericCOWFormat. > > What is the reason for even considering limiting the expressiveness? Do > you think that an additional 'optional' bool, at least for those options > that don't have a default, would be so bad in the longhand form? Or > keeping '*' even in the longhand form, as suggested below. Well, one reason is this: ## # @SchemaInfoObjectMember: # # An object member. # # @name: the member's name, as defined in the QAPI schema. # # @type: the name of the member's type. # # @default: default when used as command parameter. # If absent, the parameter is mandatory. # If present, the value must be null. The parameter is # optional, and behavior when it's missing is not specified # here. # Future extension: if present and non-null, the parameter # is optional, and defaults to this value. # # Since: 2.5 ## If we want to be able to express the difference between "behavior when absent is not specified here" and "absent behaves like value null", then we need to somehow add that bit of information here. Could use a feature. Features are not yet implemented for members, but we need them anyway. >> > If that means that keeping '*' in the longhand form of >> > optional members (whether or not those members have a default value), >> > then so be it. >> >> I believe both >> >> '*KEY': { 'type': ARG': 'default': null } >> >> and >> >> 'KEY': { 'type': ARG': 'default': null } >> >> are viable longhand forms for '*KEY': 'ARG'. >> >> I prefer the latter, but I'm open to arguments. > > If you go for the former, then you certainly want to use absent > 'default' to indicate no default, and allow a QNull default with > 'default': null. > > The only reason to abuse 'default': null for no default is that you > can't distinguish optional and non-optional if you use 'KEY' for both > instead of 'KEY' for mandatory and '*KEY' for optional. > > So while I understand and to some degree share your dislike for the '*' > prefix, I think I cast my pragmatic vote for: > > mandatory: 'KEY': { 'type': 'ARG' } > optional without a default: '*KEY': { 'type': 'ARG' } > optional with QNull default: '*KEY': { 'type': 'ARG', 'default': null } The last one could also be 'KEY': { 'type': 'ARG', 'default': null } without loss of expressiveness. Differently ugly. Here's yet another idea. For the "absent is not specified here" case, use 'KEY': { 'type': 'ARG', optional: true } '*KEY': 'ARG' For the "absent defaults to DEFVAL" case, use 'KEY': { 'type': 'ARG', optional: true, 'default': DEFVAL } 'KEY': { 'type': 'ARG', 'default': DEFVAL }
Am 22.11.2019 um 15:40 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 22.11.2019 um 08:29 hat Markus Armbruster geschrieben: > >> > At any rate, your counterpoint is taken - whatever we pick, we'll want > >> > to make sure that introspection can expose semantics, and whether we > >> > can make '*' redundant with some other form of longhand in the qapi > >> > language is in part determined by whether we also reflect that through > >> > introspection. > >> > >> Introspection has the true member name, without the '*' prefix. > >> > >> We'll also want to avoid unnecessary compromises on QAPI schema > >> expressiveness. If we use null to mean "schema does not specify > >> behavior when member is absent", we can't use it to mean "absent member > >> behaves like the value null". A bit of a blemish, but I think it's a > >> tolerable one. > > > > If you want an example for an option that defaults to null, take the > > backing option of BlockdevOptionsGenericCOWFormat. > > > > What is the reason for even considering limiting the expressiveness? Do > > you think that an additional 'optional' bool, at least for those options > > that don't have a default, would be so bad in the longhand form? Or > > keeping '*' even in the longhand form, as suggested below. > > Well, one reason is this: > > ## > # @SchemaInfoObjectMember: > # > # An object member. > # > # @name: the member's name, as defined in the QAPI schema. > # > # @type: the name of the member's type. > # > # @default: default when used as command parameter. > # If absent, the parameter is mandatory. > # If present, the value must be null. The parameter is > # optional, and behavior when it's missing is not specified > # here. > # Future extension: if present and non-null, the parameter > # is optional, and defaults to this value. > # > # Since: 2.5 > ## > > If we want to be able to express the difference between "behavior when > absent is not specified here" and "absent behaves like value null", then > we need to somehow add that bit of information here. > > Could use a feature. Features are not yet implemented for members, but > we need them anyway. That definition wasn't a great idea, I'm afraid. :-( But "default is QNull" is still acceptable behaviour for "not specified" if the client doesn't need to know what the default is. > >> > If that means that keeping '*' in the longhand form of > >> > optional members (whether or not those members have a default value), > >> > then so be it. > >> > >> I believe both > >> > >> '*KEY': { 'type': ARG': 'default': null } > >> > >> and > >> > >> 'KEY': { 'type': ARG': 'default': null } > >> > >> are viable longhand forms for '*KEY': 'ARG'. > >> > >> I prefer the latter, but I'm open to arguments. > > > > If you go for the former, then you certainly want to use absent > > 'default' to indicate no default, and allow a QNull default with > > 'default': null. > > > > The only reason to abuse 'default': null for no default is that you > > can't distinguish optional and non-optional if you use 'KEY' for both > > instead of 'KEY' for mandatory and '*KEY' for optional. > > > > So while I understand and to some degree share your dislike for the '*' > > prefix, I think I cast my pragmatic vote for: > > > > mandatory: 'KEY': { 'type': 'ARG' } > > optional without a default: '*KEY': { 'type': 'ARG' } > > optional with QNull default: '*KEY': { 'type': 'ARG', 'default': null } > > The last one could also be 'KEY': { 'type': 'ARG', 'default': null } > without loss of expressiveness. > > Differently ugly. Not loss of expressiveness, but loss of consistency. > Here's yet another idea. For the "absent is not specified here" case, > use > > 'KEY': { 'type': 'ARG', optional: true } > '*KEY': 'ARG' > > For the "absent defaults to DEFVAL" case, use > > 'KEY': { 'type': 'ARG', optional: true, 'default': DEFVAL } > 'KEY': { 'type': 'ARG', 'default': DEFVAL } I assume this means: 'optional' defaults to true if 'default' is present, and to false if 'default' is absent. (It's an example of a default that can't be expressed in the schema.) Works for me. Kevin
diff --git a/qapi/introspect.json b/qapi/introspect.json index 1843c1cb17..db703135f9 100644 --- a/qapi/introspect.json +++ b/qapi/introspect.json @@ -198,11 +198,10 @@ # # @default: default when used as command parameter. # If absent, the parameter is mandatory. -# If present, the value must be null. The parameter is -# optional, and behavior when it's missing is not specified -# here. -# Future extension: if present and non-null, the parameter -# is optional, and defaults to this value. +# If present and null, the parameter is optional, and +# behavior when it's missing is not specified here. +# If present and non-null, the parameter is optional, and +# defaults to this value. # # Since: 2.5 ## diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index b929e07be4..6c407cd4ba 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -35,7 +35,7 @@ def gen_call(name, arg_type, boxed, ret_type): elif arg_type: assert not arg_type.variants for memb in arg_type.members: - if memb.optional: + if memb.optional and memb.default is None: argstr += 'arg.has_%s, ' % c_name(memb.name) argstr += 'arg.%s, ' % c_name(memb.name) diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index c6754a5856..8c57d0c67a 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -14,6 +14,7 @@ from __future__ import print_function from contextlib import contextmanager import errno +import math import os import re import string @@ -800,6 +801,136 @@ def check_if(expr, info): check_if_str(ifcond, info) +def check_value_str(info, value): + return 'g_strdup(%s)' % to_c_string(value) if type(value) is str else False + +def check_value_number(info, value): + if type(value) is not float: + return False + if math.isinf(value): + return 'INFINITY' if value > 0 else '-INFINITY' + elif math.isnan(value): + return 'NAN' + else: + return '%.16e' % value + +def check_value_bool(info, value): + if type(value) is not bool: + return False + return 'true' if value else 'false' + +def is_int_type(value): + if type(value) is int: + return True + # 'long' does not exist in Python 3 + try: + if type(value) is long: + return True + except NameError: + pass + + return False + +def gen_check_value_int(bits): + def check_value_int(info, value): + if not is_int_type(value) or \ + value < -(2 ** (bits - 1)) or value >= 2 ** (bits - 1): + return False + if bits > 32: + return '%ill' % value + else: + return '%i' % value + + return check_value_int + +def gen_check_value_uint(bits): + def check_value_uint(info, value): + if not is_int_type(value) or value < 0 or value >= 2 ** bits: + return False + if bits > 32: + return '%uull' % value + elif bits > 16: + return '%uu' % value + else: + return '%u' % value + + return check_value_uint + +# Check whether the given value fits the given QAPI type. +# If so, return a C representation of the value (pointers point to +# newly allocated objects). +# Otherwise, raise an exception. +def check_value(info, qapi_type, value): + builtin_type_checks = { + 'str': check_value_str, + 'int': gen_check_value_int(64), + 'number': check_value_number, + 'bool': check_value_bool, + 'int8': gen_check_value_int(8), + 'int16': gen_check_value_int(16), + 'int32': gen_check_value_int(32), + 'int64': gen_check_value_int(64), + 'uint8': gen_check_value_uint(8), + 'uint16': gen_check_value_uint(16), + 'uint32': gen_check_value_uint(32), + 'uint64': gen_check_value_uint(64), + 'size': gen_check_value_uint(64), + } + + # Cannot support null because that would require a value of "None" + # (which is reserved for no default) + unsupported_builtin_types = ['null', 'any', 'QType'] + + if type(qapi_type) is list: + has_list = True + qapi_type = qapi_type[0] + elif qapi_type.endswith('List'): + has_list = True + qapi_type = qapi_type[:-4] + else: + has_list = False + + if has_list: + if value == []: + return 'NULL' + else: + raise QAPISemError(info, + "Support for non-empty lists as default values has not been " \ + "implemented yet: '{}'".format(value)) + + if qapi_type in builtin_type_checks: + c_val = builtin_type_checks[qapi_type](info, value) + if not c_val: + raise QAPISemError(info, + "Value '{}' does not match type {}".format(value, qapi_type)) + return c_val + + if qapi_type in unsupported_builtin_types: + raise QAPISemError(info, + "Cannot specify values for type %s" % qapi_type) + + if qapi_type in enum_types: + if not check_value_str(info, value): + raise QAPISemError(info, + "Enum values must be strings, but '{}' is no string" \ + .format(value)) + + enum_values = enum_types[qapi_type]['data'] + for ev in enum_values: + if ev['name'] == value: + return c_enum_const(qapi_type, value, + enum_types[qapi_type].get('prefix')) + + raise QAPISemError(info, + "Value '{}' does not occur in enum {}".format(value, qapi_type)) + + # TODO: Support alternates + + raise QAPISemError(info, + "Cannot specify values for type %s (not built-in or an enum)" % + qapi_type) + + def check_type(info, source, value, allow_array=False, allow_dict=False, allow_optional=False, allow_metas=[]): @@ -842,15 +973,22 @@ def check_type(info, source, value, allow_array=False, if c_name(key, False) == 'u' or c_name(key, False).startswith('has_'): raise QAPISemError(info, "Member of %s uses reserved name '%s'" % (source, key)) - # Todo: allow dictionaries to represent default values of - # an optional argument. + check_known_keys(info, "member '%s' of %s" % (key, source), - arg, ['type'], ['if']) + arg, ['type'], ['if', 'default']) check_type(info, "Member '%s' of %s" % (key, source), arg['type'], allow_array=True, allow_metas=['built-in', 'union', 'alternate', 'struct', 'enum']) + if 'default' in arg: + if key[0] != '*': + raise QAPISemError(info, + "'%s' is not optional, so it cannot have a default value" % + key) + + check_value(info, arg['type'], arg['default']) + def check_command(expr, info): name = expr['command'] @@ -1601,13 +1739,14 @@ class QAPISchemaFeature(QAPISchemaMember): class QAPISchemaObjectTypeMember(QAPISchemaMember): - def __init__(self, name, typ, optional, ifcond=None): + def __init__(self, name, typ, optional, ifcond=None, default=None): QAPISchemaMember.__init__(self, name, ifcond) assert isinstance(typ, str) assert isinstance(optional, bool) self._type_name = typ self.type = None self.optional = optional + self.default = default def check(self, schema): assert self.owner @@ -1917,7 +2056,7 @@ class QAPISchema(object): name, info, doc, ifcond, self._make_enum_members(data), prefix)) - def _make_member(self, name, typ, ifcond, info): + def _make_member(self, name, typ, ifcond, default, info): optional = False if name.startswith('*'): name = name[1:] @@ -1925,10 +2064,11 @@ class QAPISchema(object): if isinstance(typ, list): assert len(typ) == 1 typ = self._make_array_type(typ[0], info) - return QAPISchemaObjectTypeMember(name, typ, optional, ifcond) + return QAPISchemaObjectTypeMember(name, typ, optional, ifcond, default) def _make_members(self, data, info): - return [self._make_member(key, value['type'], value.get('if'), info) + return [self._make_member(key, value['type'], value.get('if'), + value.get('default'), info) for (key, value) in data.items()] def _def_struct_type(self, expr, info, doc): @@ -1951,7 +2091,7 @@ class QAPISchema(object): typ = self._make_array_type(typ[0], info) typ = self._make_implicit_object_type( typ, info, None, self.lookup_type(typ), - 'wrapper', [self._make_member('data', typ, None, info)]) + 'wrapper', [self._make_member('data', typ, None, None, info)]) return QAPISchemaObjectTypeVariant(case, typ, ifcond) def _def_union_type(self, expr, info, doc): @@ -2234,6 +2374,15 @@ def to_c_string(string): return result +# Translates a value for the given QAPI type to its C representation. +# The caller must have called check_value() during parsing to be sure +# that the given value fits the type. +def c_value(qapi_type, value): + pseudo_info = {'file': '(generator bug)', 'line': 0, 'parent': None} + # The caller guarantees this does not raise an exception + return check_value(pseudo_info, qapi_type, value) + + def guardstart(name): return mcgen(''' #ifndef %(name)s @@ -2356,7 +2505,7 @@ def build_params(arg_type, boxed, extra=None): for memb in arg_type.members: ret += sep sep = ', ' - if memb.optional: + if memb.optional and memb.default is None: ret += 'bool has_%s, ' % c_name(memb.name) ret += '%s %s' % (memb.type.c_param_type(), c_name(memb.name)) diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 5fc0fc7e06..78a9052738 100755 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -139,13 +139,29 @@ def texi_enum_value(value, desc, suffix): value.name, desc, texi_if(value.ifcond, prefix='@*')) +def doc_value(value): + if value is True: + return 'true' + elif value is False: + return 'false' + elif value is None: + return 'null' + else: + return '{}'.format(value) + def texi_member(member, desc, suffix): """Format a table of members item for an object type member""" typ = member.type.doc_type() membertype = ': ' + typ if typ else '' + + optional_info = '' + if member.default is not None: + optional_info = ' (optional, default: %s)' % doc_value(member.default) + elif member.optional: + optional_info = ' (optional)' + return '@item @code{%s%s}%s%s\n%s%s' % ( - member.name, membertype, - ' (optional)' if member.optional else '', + member.name, membertype, optional_info, suffix, desc, texi_if(member.ifcond, prefix='@*')) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 572e0b8331..7d73020a42 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -159,7 +159,7 @@ const QLitObject %(c_name)s = %(c_string)s; def _gen_member(self, member): ret = {'name': member.name, 'type': self._use_type(member.type)} if member.optional: - ret['default'] = None + ret['default'] = member.default if member.ifcond: ret = (ret, {'if': member.ifcond}) return ret diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 3edd9374aa..46a6d33379 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -44,7 +44,7 @@ def gen_struct_members(members): ret = '' for memb in members: ret += gen_if(memb.ifcond) - if memb.optional: + if memb.optional and memb.default is None: ret += mcgen(''' bool has_%(c_name)s; ''', diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 484ebb66ad..0960e25a25 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -40,10 +40,14 @@ def gen_visit_object_members(name, base, members, variants): void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) { Error *err = NULL; - ''', c_name=c_name(name)) + if len([m for m in members if m.default is not None]) > 0: + ret += mcgen(''' + bool has_optional; +''') + if base: ret += mcgen(''' visit_type_%(c_type)s_members(v, (%(c_type)s *)obj, &err); @@ -53,13 +57,28 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) ''', c_type=base.c_name()) + ret += mcgen(''' + +''') + for memb in members: ret += gen_if(memb.ifcond) if memb.optional: + if memb.default is not None: + optional_target = 'has_optional' + # Visitors other than the input visitor do not have to implement + # .optional(). Therefore, we have to initialize has_optional. + # Initialize it to true, because the field's value is always + # present when using any visitor but the input visitor. + ret += mcgen(''' + has_optional = true; +''') + else: + optional_target = 'obj->has_' + c_name(memb.name) ret += mcgen(''' - if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) { + if (visit_optional(v, "%(name)s", &%(opt_target)s)) { ''', - name=memb.name, c_name=c_name(memb.name)) + name=memb.name, opt_target=optional_target) push_indent() ret += mcgen(''' visit_type_%(c_type)s(v, "%(name)s", &obj->%(c_name)s, &err); @@ -69,7 +88,16 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) ''', c_type=memb.type.c_name(), name=memb.name, c_name=c_name(memb.name)) - if memb.optional: + if memb.default is not None: + pop_indent() + ret += mcgen(''' + } else { + obj->%(c_name)s = %(c_value)s; + } +''', + c_name=c_name(memb.name), + c_value=c_value(memb._type_name, memb.default)) + elif memb.optional: pop_indent() ret += mcgen(''' } @@ -287,6 +315,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): self._add_system_module(None, ' * Built-in QAPI visitors') self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" +#include <math.h> #include "qapi/error.h" #include "qapi/qapi-builtin-visit.h" ''')) @@ -302,6 +331,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): visit = self._module_basename('qapi-visit', name) self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" +#include <math.h> #include "qapi/error.h" #include "qapi/qmp/qerror.h" #include "%(visit)s.h"
With this change, it is possible to give default values for struct members, as follows: What you had to do so far: # @member: Some description, defaults to 42. { 'struct': 'Foo', 'data': { '*member': 'int' } } What you can do now: { 'struct': 'Foo', 'data': { '*member': { 'type': 'int', 'default': 42 } } On the C side, this change would remove Foo.has_member, because Foo.member is always valid now. The input visitor deals with setting it. (Naturally, this means that such defaults are useful only for input parameters.) At least three things are left unimplemented: First, support for alternate data types. This is because supporting them would mean having to allocate the object in the input visitor, and then potentially going through multiple levels of nested types. In any case, it would have been difficult and I do not think there is need for such support at this point. Second, support for null. The most important reason for this is that introspection already uses "'default': null" for "no default, but this field is optional". The second reason is that without support for alternate data types, there is not really a point in supporting null. Third, full support for default lists. This has a similar reason to the lack of support for alternate data types: Allocating a default list is not trivial -- unless the list is empty, which is exactly what we have support for. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qapi/introspect.json | 9 +- scripts/qapi/commands.py | 2 +- scripts/qapi/common.py | 167 +++++++++++++++++++++++++++++++++++-- scripts/qapi/doc.py | 20 ++++- scripts/qapi/introspect.py | 2 +- scripts/qapi/types.py | 2 +- scripts/qapi/visit.py | 38 ++++++++- 7 files changed, 217 insertions(+), 23 deletions(-)