Re: [PATCH v2 2/6] migration: Updated QAPI format for 'migrate' qemu monitor command

[Markus Armbruster]

Daniel P. Berrangé <berrange@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)