Re: [PATCH 1/5] migration: Updated QAPI format for 'migrate' qemu monitor command

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Mon, Dec 26, 2022 at 05:33:25AM +0000, Het Gala wrote:
> > From: Author Het Gala <het.gala@nutanix.com>
> > 
> > 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. This scheme does seem to have an issue
> > in it, i.e. double-level encoding of URIs.
> > 
> > The current patch maps existing QAPI design into a well-defined data
> > structure - 'MigrateChannel' only from the design perspective. Please note that
> > the existing 'uri' parameter is kept untouched for backward compatibility.
> > 
> > Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> > Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >  qapi/migration.json | 121 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 119 insertions(+), 2 deletions(-)
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 88ecf86ac8..753e187ce2 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1449,12 +1449,108 @@
> >  ##
> >  { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> >  
> > +##
> > +# @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' } }
> > +
> > +##
> > +# @MigrateExecAddr:
> > + #
> > + # Since 8.0
> > + ##
> > +{ 'struct': 'MigrateExecAddr',
> > +   'data' : {'exec-str': 'str' } }
> 
> Currently for 'exec:cmdstr' the 'cmdstr' part is a shell command
> that is passed
> 
>   const char *argv[] = { "/bin/sh", "-c", command, NULL };
> 
> I have a strong preference for making it possible to avoid use
> of shell when spawning commands, since much of thue time it is
> not required and has the potential to open up vulnerabilities.
> It would be nice to be able to just take the full argv directly
> IOW
> 
>  { 'struct': 'MigrateExecAddr',
>     'data' : {'argv': ['str'] } }
> 
> If the caller wants to keep life safe and simple now they can
> use
>    ["/bin/nc", "-U", "/some/sock"]
> 
> but if they still want to send it via shell, they can also do
> so
> 
>    ["/bin/sh", "-c", "...arbitrary shell script code...."]
> 
> > +
> > +##
> > +# @MigrateRdmaAddr:
> > +#
> > +# Since 8.0
> > +##
> > +{ 'struct': 'MigrateRdmaAddr',
> > +   'data' : {'rdma-str': 'str' } }
> 
> Loooking at the RDMA code it takes the str, and treats it
> as an IPv4 address:
> 
> 
>         addr = g_new(InetSocketAddress, 1);
>         if (!inet_parse(addr, host_port, NULL)) {
>             rdma->port = atoi(addr->port);
>             rdma->host = g_strdup(addr->host);
>             rdma->host_port = g_strdup(host_port);
>         } 
> 
> so we really ought to accept an InetSocketAddress struct
> directly 
> 
>  { 'struct': 'MigrateRdmaAddr',
>     'data' : {'rdma-str': 'InetSocketAddress' } }

I think that's probably the right thing to do; there is a native RDMA
addressing scheme that people occasionally (once a decade or so)
ask about but I don't think we've ever supported it.

Dave

> 
> 
> > +
> > +##
> > +# @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' } }
> > +
> > +##
> > +# @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'] }
> > +
> > +##
> > +# @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
> > +#
> > +# 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,36 @@
> >  # 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 mututally exclusive but, atleast
> 
> 
> Minor typos                                   "mutually"                "at least"
> 
> > +#    one of the two arguments should be present.
> > +#
> >  # 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" } } } } }
> > +# <- { "return": {} }
> > +#
> > +# -> { "execute": "migrate",
> > +#      "arguments": { "channel": { "channeltype": "main",
> > +#                                  "addr": { "transport": "exec",
> > +#                                            "exec-str": "/tmp/exec" } } } }
> 
> Will need updating given my feedback above
> 
> > +# <- { "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' } }
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK