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

[Het Gala]

On 09/02/23 1:47 am, Eric Blake wrote:
> On Wed, Feb 08, 2023 at 09:35:56AM +0000, Het Gala wrote:
> > Existing 'migrate' QAPI design enforces transport mechanism, ip address
> > of destination interface and corresponding port number in the form
> > of a unified string 'uri' parameter for initiating a migration stream.
> > This scheme has a significant flaw in it - double encoding of existing
> > URIs to extract migration info.
> >
> > +##
> > +# @MigrateTransport:
> > +#
> > +# The supported communication transport mechanisms for migration
> > +#
> > +# @socket: Supported communication type between two devices for migration.
> > +#          Socket is able to cover all of 'tcp', 'unix', 'vsock' and
> > +#          'fd' already
> > +#
> > +# @exec: Supported communication type to redirect migration stream into file.
> > +#
> > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > +#
> > +# Since 8.0
> > +##
> > +{ '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' } }
> Here, you use 'socket-type',...
Yes, I wanted a suggestion here actually. Will 'data' instead of 
'socket-type' be the right fit ? It will also be consistent with exec 
and rdma if changed to 'data'.
> > +
> > +##
> > +# @MigrateExecAddr:
> > + #
> > + # Since 8.0
> > + ##
> > +{ 'struct': 'MigrateExecAddr',
> > +   'data' : {'data': ['str'] } }
> Inconsistent on whether you have a space before :.  Most of our qapi
> files prefer the layout:
>
> 'key': 'value'
>
> that is, no space before, one space after.  It doesn't affect
> correctness, but a consistent visual style is worth striving for.
Okay, thanks Eric for pointing it out. Will make sure I follow this 
going forward.
> > +
> > +##
> > +# @MigrateRdmaAddr:
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateRdmaAddr',
> > +   'data' : {'data': 'InetSocketAddress' } }
> ...while these branches supply everything else under 'data'. Also,
> while you documented @socket-type above, you did not document @data in
> either of these two types.  [1]
Ack. In that case, I feel it would be better if I change from 
'socket-type' to 'data' to keep consistency among the QAPI.
> > +
> > +##
> > +# @MigrateAddress:
> > +#
> > +# The options available for communication transport mechanisms for migration
> > +#
> > +# Since 8.0
> > +##
> > +{ 'union' : 'MigrateAddress',
> > +  'base' : { 'transport' : 'MigrateTransport'},
> > +  'discriminator' : 'transport',
> > +  'data' : {
> > +    'socket' : 'MigrateSocketAddr',
> > +    'exec' : 'MigrateExecAddr',
> > +    'rdma': 'MigrateRdmaAddr' } }
> Another example of inconsistent spacing around :.
>
> I'm guessing the reason you didn't go with 'socket': 'SocketAddress'
> is that SocketAddress is itself a discriminated union, and Markus does
> not yet have the QAPI generator wired up to support one union as a
> branch of another larger union?  It leads to extra nesting on the wire
> [2]
Yes Eric. I did search if it is possible for a union inside a branch of 
union. That's the reason, I had to choose this path for 'socket' and 
'rdma', and to keep consistency, I did the same with 'exec' too.
> > +
> > +##
> > +# @MigrateChannelType:
> > +#
> > +# The supported options for migration channel type requests
> > +#
> > +# @main: Support request for main outbound migration control channel
> > +#
> > +# Since 8.0
> > +##
> > +{ 'enum': 'MigrateChannelType',
> > +  'data': [ 'main'] }
> A different spacing issue: most arrays in QAPI either have spaces at
> both ends (as in [ 'string' ]) or neither (as in ['string']).  Here,
> it looks lopsided with space at the front but not the back.
Ack Eric. Thanks for pointing it out. Will take care about this small 
issues from next time.
> > +
> > +##
> > +# @MigrateChannel:
> > +#
> > +# Information regarding migration Channel-type for transferring packets,
> > +# source and corresponding destination interface for socket connection
> > +# and number of multifd channels over the interface.
> > +#
> > +# @channeltype: Name of Channel type for transfering packet information
> > +#
> > +# @addr: SocketAddress of destination interface
> More than just a SocketAddress, per the discriminated union type defined above.
Yes, infact one of the branches MigrateChannel. Ack.
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateChannel',
> > +  'data' : {
> > +       'channeltype' : 'MigrateChannelType',
> > +       'addr' : 'MigrateAddress' } }
> > +
> >   ##
> >   # @migrate:
> >   #
> >   # Migrates the current running guest to another Virtual Machine.
> >   #
> >   # @uri: the Uniform Resource Identifier of the destination VM
> > +#       for migration thread
> > +#
> > +# @channel: Struct containing migration channel type, along with all
> > +#           the details of destination interface required for initiating
> > +#           a migration stream.
> >   #
> >   # @blk: do block migration (full disk copy)
> >   #
> > @@ -1479,15 +1575,46 @@
> >   # 3. The user Monitor's "detach" argument is invalid in QMP and should not
> >   #    be used
> >   #
> > +# 4. The uri argument should have the Uniform Resource Identifier of default
> > +#    destination VM. This connection will be bound to default network
> > +#
> > +# 5. Both 'uri' and 'channel' arguments, are mutually exclusive but, at least
> > +#    one of the two arguments should be present.
> Grammar suggestion:
>
> 'uri' and 'channel' are mutually exclusive; exactly one of the two
> should be present.
Ack.
> > +#
> >   # Example:
> >   #
> >   # -> { "execute": "migrate", "arguments": { "uri": "tcp:0:4446" } }
> >   # <- { "return": {} }
> >   #
> > +# -> { "execute": "migrate",
> > +#      "arguments": {
> > +#          "channel": { "channeltype": "main",
> > +#                        "addr": { "transport": "socket",
> > +#                                  "socket-type": { "type': "inet',
> > +#                                                   "host": "10.12.34.9",
> > +#                                                   "port": "1050" } } } } }
> [2] This is the extra nesting that occurs because of the
> 'socket-type':'MigrateSocketAddr' above; getting rid of the nesting
> would require 'socket-type':'SocketAddress', but QAPI would need to
> support that first.
Yes Eric, excatly.
> > +# <- { "return": {} }
> > +#
> > +# -> { "execute": "migrate",
> > +#      "arguments": {
> > +#          "channel": { "channeltype": "main",
> > +#                       "addr": { "transport": "exec",
> > +#                                 "exec": ["/bin/nc", "-U",
> > +#                                          "/some/sock" ] } } } }
> Another lopsided spacing in [].
Ack.
> > +# <- { "return": {} }
> > +#
> > +# -> { "execute": "migrate",
> > +#      "arguments": {
> > +#          "channel": { "channeltype": "main",
> > +#                       "addr": { "transport": "rdma",
> > +#                                 "rdma": { "host": "10.12.34.9",
> > +#                                           "port": "1050" } } } } }
> [1] These examples look wrong; above, the schema named the nesting as 'data', 
> rather than 'exec' or 'rdma'.
Yes, instead of 'rdma' or 'exec', it should be replaced by 'data' here 
in the examples. Ack.
> > +# <- { "return": {} }
> > +#
> >   ##
> >   { 'command': 'migrate',
> > -  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool',
> > -           '*detach': 'bool', '*resume': 'bool' } }
> > +  'data': {'*uri': 'str', '*channel': 'MigrateChannel', '*blk': 'bool',
> > +           '*inc': 'bool', '*detach': 'bool', '*resume': 'bool' } }
> But overall, I'm a fan of using a more type-accurate description of
> the channel than the open-coded 'uri':'str'.
Yes, 'uri':'str' is kept for backward compatibility right now. This will 
be depricated in later stages.
Regards,
Het Gala.