Daniel P. Berrangé <berra...@redhat.com> writes: > On Thu, Feb 09, 2023 at 10:23:43AM +0000, Daniel P. Berrangé wrote:
[...] >> I don't know the backstory on this limitation. Is it something that >> is very difficult to resolve ? I think it is highly desirable to have >> 'socket': 'SocketAddress' here. It would be a shame to introduce this >> better migration API design and then have it complicated by a possibly >> short term limitation of QAPI. > > So to understand this better I did this change on top of Het's > patch: > > diff --git a/qapi/migration.json b/qapi/migration.json > index 79acfcfe4e..bbc3e66ad6 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -1467,18 +1467,6 @@ > { 'enum': 'MigrateTransport', > 'data': ['socket', 'exec', 'rdma'] } > > -## > -# @MigrateSocketAddr: > -# > -# To support different type of socket. > -# > -# @socket-type: Different type of socket connections. > -# > -# Since 8.0 > -## > -{ 'struct': 'MigrateSocketAddr', > - 'data': {'socket-type': 'SocketAddress' } } > - > ## > # @MigrateExecAddr: > # > @@ -1487,14 +1475,6 @@ > { 'struct': 'MigrateExecAddr', > 'data' : {'data': ['str'] } } > > -## > -# @MigrateRdmaAddr: > -# > -# Since 8.0 > -## > -{ 'struct': 'MigrateRdmaAddr', > - 'data' : {'data': 'InetSocketAddress' } } > - > ## > # @MigrateAddress: > # > @@ -1506,9 +1486,9 @@ > 'base' : { 'transport' : 'MigrateTransport'}, > 'discriminator' : 'transport', > 'data' : { > - 'socket' : 'MigrateSocketAddr', > + 'socket' : 'SocketAddress', > 'exec' : 'MigrateExecAddr', > - 'rdma': 'MigrateRdmaAddr' } } > + 'rdma': 'InetSocketAddress' } } > > ## > # @MigrateChannelType: > > > That results in a build error > > /home/berrange/src/virt/qemu/scripts/qapi-gen.py: In file included from > ../qapi/qapi-schema.json:79: > ../qapi/migration.json: In union 'MigrateAddress': > ../qapi/migration.json:1505: branch 'socket' cannot use union type > 'SocketAddress' > > To understand what the limitation of QAPI generation is, I blindly > disabled this check > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index cd8661125c..cb5c0385bd 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -653,7 +653,7 @@ def check(self, schema, seen): > "branch '%s' is not a value of %s" > % (v.name, self.tag_member.type.describe())) > if (not isinstance(v.type, QAPISchemaObjectType) > - or v.type.variants): > + or v.type.variants) and False: > raise QAPISemError( > self.info, > "%s cannot use %s" > @@ -664,7 +664,8 @@ def check_clash(self, info, seen): > for v in self.variants: > # Reset seen map for each variant, since qapi names from one > # branch do not affect another branch > - v.type.check_clash(info, dict(seen)) > + #v.type.check_clash(info, dict(seen)) > + pass > > > class QAPISchemaMember: > > > After doing that, the QAPI code generated handled the union-inside-union > case without any problems that I can see. The generated code looks sane > and it compiles correctly. As you discovered, the code was designed to treat structs and unions the same. But there are holes. > So is this actually just a case of overly strict input validation in > the QAPI schema parser ? If so, what's the correct way to adapt the > checks to permit this usage. The check you disabled guards holes. There's at least this one in QAPISchemaObjectType.check_clash(): # Check that the members of this type do not cause duplicate JSON members, # and update seen to track the members seen so far. Report any errors # on behalf of info, which is not necessarily self.info def check_clash(self, info, seen): assert self._checked assert not self.variants # not implemented for m in self.members: m.check_clash(info, seen)