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

[Daniel P . Berrangé]

On Wed, Feb 08, 2023 at 02:17:12PM -0600, 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.
> > 
> > The current patch maps QAPI uri design onto well defined MigrateChannel
> > struct. This modified QAPI helps in preventing multi-level uri
> > encodings ('uri' parameter is kept 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 | 131 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 129 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index c84fa10e86..79acfcfe4e 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' } }
> 
> Here, you use 'socket-type',...
> 
> > +
> > +##
> > +# @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.
> 
> > +
> > +##
> > +# @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]
> 
> > +
> > +##
> > +# @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]

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.

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 :|