Re: [PATCH v5 1/9] migration: introduced 'MigrateAddress' in QAPI for migration wire protocol.

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, May 30, 2023 at 02:26:23PM +0530, Het Gala wrote:
> 
> On 30/05/23 1:26 pm, Markus Armbruster wrote:
> > Het Gala <het.gala@nutanix.com> writes:
> > 
> > > On 30/05/23 12:28 pm, Markus Armbruster wrote:
> > > > Het Gala<het.gala@nutanix.com>  writes:
> > > > 
> > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote:
> > [...]
> > 
> > > > > > Aside: a more powerful type system would let us extend SocketAddress
> > > > > > with additional variants instead of wrapping it in a union.
> > > > > Markus, what do you mean by additional variants here in context of socket? Can you give a small example.
> > > > As is, we have a nest of two unions:
> > > > 
> > > > * The outer union has branches @socket, @exec, @rdma.
> > > > 
> > > > * Branch @socket is the inner union, it has branches @inet, @unix, ...
> > > > 
> > > > A more powerful type system would let us extend SocketAddress instead,
> > > > so MigrateAddress has everything SocketAddress has, plus additional
> > > > branches @exec, @rdma.  Naturally, the type of the discriminator also
> > > > needs to be extended, so that it has everything SocketAddress's
> > > > discriminator type has, plus additional members @exec, @rdma.
> > > Okay, so you mean something like :
> > > 
> > > +# Since 8.1
> > > +##
> > > +{ 'union': 'MigrateAddress',
> > > +  'base': { 'transport' : 'MigrateTransport'},
> > > +  'discriminator': 'transport',
> > > +  'data': {
> > > +    'inet': 'InetSocketAddress',
> > > +    'unix': 'UnixSocketAddress',
> > > +    'vsock': 'VsockSocketAddress',
> > > +    'fd': 'str',
> > > +    'exec': 'MigrateExecCommand',
> > > +    'rdma': 'InetSocketAddress' } }
> > > 
> > > Even I agree that directly leveraging this is the best option, but then wouldn't it introduce redundancy ? we would not be able to leverage socket union right in that case or am I missing something.
> > Yes, there's redundancy, due to QAPI's insufficient expressive power.
> > 
> > Is the cleaner external interface worth the (internal) redundancy, and
> > possibly coding complications that go with it?
> 
> Honestly, I would like to have it this way, where the user is aware of all
> the transport mechanisms available. But I guess for external interface
> problem statement, the migration code flow has similar path for
> SocketAddress and non-socketAddress (exec and rdma). So even if on the QAPI
> side we express explicitly all the transport mechanisms, while implementing
> it we either would need to brinf them under a single umbrella. For now, I
> would keep the implementation as it is (union inside a union) but would want
> to have a more powerful and better appraoch out there if possible.

IMHO, unpacking the SocketAddress types into the two level is not something
we need to do. The main (perhaps even only) reason it arises as a design
question is because historically the migration public interface has NOT
used SocketAddress, and exposed unix/inet/vsock as distinct top level 
options, and this taints our thoughts.

If the "migrate" command didn't already exist and we were adding it now
IMHO we would just expose 'socket': 'SocketAdress' as a top level type,
in parallel with 'exec' and 'rdma', and not even discuss the idea of
unpacking 'SocketAddress'.

IMHO the compelling reason to NOT unpack 'SocketAddress' is that it lets
the migration code seemlessly benefit from any new 'SocketAddress' types
that gets implemented. eg consider when 'vsock' was added to QEMU. Any
command that took a 'SocketAddress' parameter needed no QAPI changes
and usually no code changes either.

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