Daniel P. Berrangé <[email protected]> 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)
