Eric Blake <ebl...@redhat.com> writes:
> There's no reason to do two malloc's for an alternate type visiting
> a QAPI struct; let's just inline the struct directly as the C union
> branch of the struct.
>
> Surprisingly, no clients were actually using the struct member prior
> to this patch; some testsuite coverage is added to avoid future
> regressions.
>
> Ultimately, we want to do the same treatment for QAPI unions, but
> as that touches a lot more client code, it is better as a separate
> patch. So in the meantime, I had to hack in a way to test if we
> are visiting an alternate type, within qapi-types:gen_variants();
> the hack is possible because an earlier patch guaranteed that all
> alternates have at least two branches, with at most one object
> branch; the hack will go away in a later patch.
Suggest:
Ultimately, we want to do the same treatment for QAPI unions, but as
that touches a lot more client code, it is better as a separate patch.
The two share gen_variants(), and I had to hack in a way to test if we
are visiting an alternate type: look for a non-object branch. This
works because alternates have at least two branches, with at most one
object branch, and unions have only object branches. The hack will go
away in a later patch.
> The generated code difference to qapi-types.h is relatively small,
> made possible by a new c_type(is_member) parameter in qapi.py:
Let's drop the "made possible" clause here.
>
> | struct BlockdevRef {
> | QType type;
> | union { /* union tag is @type */
> | void *data;
> |- BlockdevOptions *definition;
> |+ BlockdevOptions definition;
> | char *reference;
> | } u;
> | };
>
> meanwhile, in qapi-visit.h, we create a new visit_type_alternate_Foo(),
> comparable to visit_type_implicit_Foo():
>
> |+static void visit_type_alternate_BlockdevOptions(Visitor *v, const char
> *name, BlockdevOptions *obj, Error **errp)
> |+{
> |+ Error *err = NULL;
> |+
> |+ visit_start_struct(v, name, NULL, 0, &err);
> |+ if (err) {
> |+ goto out;
> |+ }
> |+ visit_type_BlockdevOptions_fields(v, obj, &err);
> |+ error_propagate(errp, err);
> |+ err = NULL;
> |+ visit_end_struct(v, &err);
> |+out:
> |+ error_propagate(errp, err);
> |+}
This is in addition to visit_type_BlockdevOptions(), so we need another
name.
I can't quite see how the function is tied to alternates, though.
>
> and use it like this:
>
> | switch ((*obj)->type) {
> | case QTYPE_QDICT:
> |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err);
> |+ visit_type_alternate_BlockdevOptions(v, name,
> &(*obj)->u.definition, &err);
Let's compare the two functions. First visit_type_BlockdevOptions():
void visit_type_BlockdevOptions(Visitor *v,
const char *name,
BlockdevOptions **obj,
Error **errp)
{
Error *err = NULL;
visit_start_struct(v, name, (void **)obj, sizeof(BlockdevOptions),
&err);
if (err) {
goto out;
}
if (!*obj) {
goto out_obj;
}
visit_type_BlockdevOptions_fields(v, *obj, &err);
if (err) {
goto out_obj;
}
if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) {
goto out_obj;
}
switch ((*obj)->driver) {
case BLOCKDEV_DRIVER_ARCHIPELAGO:
visit_type_implicit_BlockdevOptionsArchipelago(v,
&(*obj)->u.archipelago, &err);
break;
[All the other cases...]
default:
abort();
}
out_obj:
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
Now visit_type_alternate_BlockdevOptions(), with differences annotated
with //:
static void visit_type_alternate_BlockdevOptions(Visitor *v,
const char *name,
BlockdevOptions *obj, // one * less
Error **errp)
{
Error *err = NULL;
visit_start_struct(v, name, NULL, 0, &err); // NULL instead of &obj
// suppresses malloc
if (err) {
goto out;
}
// null check dropped (obj can't be null)
visit_type_BlockdevOptions_fields(v, obj, &err);
// visit_start_union() + switch dropped
error_propagate(errp, err);
err = NULL;
visit_end_struct(v, &err);
out:
error_propagate(errp, err);
}
Why can we drop visit_start_union() + switch?
> | break;
> | case QTYPE_QSTRING:
> | visit_type_str(v, name, &(*obj)->u.reference, &err);
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v10: new patch
> ---
> scripts/qapi-types.py | 10 ++++++-
> scripts/qapi-visit.py | 49
> +++++++++++++++++++++++++++++----
> scripts/qapi.py | 10 ++++---
> tests/test-qmp-input-visitor.c | 29 ++++++++++++++++++-
> tests/qapi-schema/qapi-schema-test.json | 2 ++
> tests/qapi-schema/qapi-schema-test.out | 2 ++
> 6 files changed, 91 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 2f23432..aba2847 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -116,6 +116,14 @@ static inline %(base)s *qapi_%(c_name)s_base(const
> %(c_name)s *obj)
>
>
> def gen_variants(variants):
> + # HACK: Determine if this is an alternate (at least one variant
> + # is not an object); unions have all branches as objects.
> + inline = False
> + for v in variants.variants:
> + if not isinstance(v.type, QAPISchemaObjectType):
> + inline = True
> + break
> +
> # FIXME: What purpose does data serve, besides preventing a union that
> # has a branch named 'data'? We use it in qapi-visit.py to decide
> # whether to bypass the switch statement if visiting the discriminator
> @@ -136,7 +144,7 @@ def gen_variants(variants):
> ret += mcgen('''
> %(c_type)s %(c_name)s;
> ''',
> - c_type=typ.c_type(),
> + c_type=typ.c_type(is_member=inline),
> c_name=c_name(var.name))
>
> ret += mcgen('''
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 9ff0337..948bde4 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,10 +15,14 @@
> from qapi import *
> import re
>
> -# visit_type_FOO_implicit() is emitted as needed; track if it has already
> +# visit_type_implicit_FOO() is emitted as needed; track if it has already
> # been output.
> implicit_structs_seen = set()
>
> +# visit_type_alternate_FOO() is emitted as needed; track if it has already
> +# been output.
> +alternate_structs_seen = set()
> +
> # visit_type_FOO_fields() is always emitted; track if a forward declaration
> # or implementation has already been output.
> struct_fields_seen = set()
> @@ -71,6 +75,35 @@ static void visit_type_implicit_%(c_type)s(Visitor *v,
> %(c_type)s **obj, Error *
> return ret
>
>
> +def gen_visit_alternate_struct(typ):
> + if typ in alternate_structs_seen:
> + return ''
> + alternate_structs_seen.add(typ)
> +
> + ret = gen_visit_fields_decl(typ)
> +
> + ret += mcgen('''
> +
> +static void visit_type_alternate_%(c_type)s(Visitor *v, const char *name,
> %(c_type)s *obj, Error **errp)
> +{
> + Error *err = NULL;
> +
> + visit_start_struct(v, name, NULL, 0, &err);
> + if (err) {
> + goto out;
> + }
> + visit_type_%(c_type)s_fields(v, obj, &err);
> + error_propagate(errp, err);
> + err = NULL;
> + visit_end_struct(v, &err);
> +out:
> + error_propagate(errp, err);
> +}
> +''',
> + c_type=typ.c_name())
> + return ret
> +
> +
> def gen_visit_struct_fields(name, base, members):
> ret = ''
>
> @@ -158,11 +191,14 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s *obj, Error
>
> def gen_visit_alternate(name, variants):
> promote_int = 'true'
> + ret = ''
> for var in variants.variants:
> if var.type.alternate_qtype() == 'QTYPE_QINT':
> promote_int = 'false'
> + if isinstance(var.type, QAPISchemaObjectType):
> + ret += gen_visit_alternate_struct(var.type)
>
> - ret = mcgen('''
> + ret += mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> @@ -178,15 +214,18 @@ void visit_type_%(c_name)s(Visitor *v, const char
> *name, %(c_name)s **obj, Error
> }
> switch ((*obj)->type) {
> ''',
> - c_name=c_name(name), promote_int=promote_int)
> + c_name=c_name(name), promote_int=promote_int)
>
> for var in variants.variants:
> + prefix = 'visit_type_'
> + if isinstance(var.type, QAPISchemaObjectType):
> + prefix += 'alternate_'
> ret += mcgen('''
> case %(case)s:
> - visit_type_%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
> + %(prefix)s%(c_type)s(v, name, &(*obj)->u.%(c_name)s, &err);
> break;
> ''',
> - case=var.type.alternate_qtype(),
> + prefix=prefix, case=var.type.alternate_qtype(),
> c_type=var.type.c_name(),
> c_name=c_name(var.name))
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4d75d75..dbb58da 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -821,7 +821,7 @@ class QAPISchemaVisitor(object):
>
>
> class QAPISchemaType(QAPISchemaEntity):
> - def c_type(self, is_param=False):
> + def c_type(self, is_param=False, is_member=False):
> return c_name(self.name) + pointer_suffix
>
> def c_null(self):
> @@ -854,7 +854,7 @@ class QAPISchemaBuiltinType(QAPISchemaType):
> def c_name(self):
> return self.name
>
> - def c_type(self, is_param=False):
> + def c_type(self, is_param=False, is_member=False):
> if is_param and self.name == 'str':
> return 'const ' + self._c_type_name
> return self._c_type_name
> @@ -888,7 +888,7 @@ class QAPISchemaEnumType(QAPISchemaType):
> # See QAPISchema._make_implicit_enum_type()
> return self.name.endswith('Kind')
>
> - def c_type(self, is_param=False):
> + def c_type(self, is_param=False, is_member=False):
> return c_name(self.name)
>
> def member_names(self):
> @@ -984,8 +984,10 @@ class QAPISchemaObjectType(QAPISchemaType):
> assert not self.is_implicit()
> return QAPISchemaType.c_name(self)
>
> - def c_type(self, is_param=False):
> + def c_type(self, is_param=False, is_member=False):
> assert not self.is_implicit()
> + if is_member:
> + return c_name(self.name)
> return QAPISchemaType.c_type(self)
To understand this, you have to peel off OO abstraction:
QAPISchemaType.c_type(self) returns c_name(self.name) + pointer_suffix.
I think we should keep it peeled off in the source code.
>
> def json_type(self):
[tests/ diff looks good]
