Less than thorough review, because I expect the necessary rebase will
require a bit of rewriting here and there.
Max Reitz <mre...@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 <mre...@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"
