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 01:02:27PM +0530, Het Gala wrote:
> 
> 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:
> > > > Het Gala<het.gala@nutanix.com>  writes:
> > > > 
> > > > > This patch introduces well defined MigrateAddress struct and its related child
> > > > > objects.
> > > > > 
> > > > > The existing argument of 'migrate' and 'migrate-incoming' QAPI - 'uri' is of
> > > > > string type. The current migration flow follows double encoding scheme for
> > > > > fetching migration parameters such as 'uri' and this is not an ideal design.
> > > > > 
> > > > > Motive for intoducing struct level design is to prevent double encoding of QAPI
> > > > > arguments, as Qemu should be able to directly use the QAPI arguments without
> > > > > any level of encoding.
> > > > > 
> > > > > Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
> > > > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > > > Reviewed-by: Juan Quintela<quintela@redhat.com>
> > > > > Reviewed-by: Daniel P. Berrangé<berrange@redhat.com>
> > > > > ---
> > > > >    qapi/migration.json | 41 +++++++++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 41 insertions(+)
> > > > > 
> > > > > diff --git a/qapi/migration.json b/qapi/migration.json
> > > > > index 179af0c4d8..c500744bb7 100644
> > > > > --- a/qapi/migration.json
> > > > > +++ b/qapi/migration.json
> > > > > @@ -1407,6 +1407,47 @@
> > > > >    ##
> > > > >    { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
> > > > >    +##
> > > > > +# @MigrateTransport:
> > > > I'd prefer MigrationTransport, because "migration" is a noun, while
> > > > migrate is a verb.  Verbs are for commands.  For types we use nouns.
> > > > More of the same below, not noting it again.
> > > > 
> > > > Actually, I'd prefer MigrationAddressType, because it's purpose is to
> > > > serve as discriminator type in union MigrationAddress.
> > > > 
> > > Okay got it. I kept it Transport as they are different transport mechanisms. But 'MigrationAddressType' looks cleaner and comaptible with 'MigrateAddress' union too. Will change that
> > Transport isn't bad, but I think a type that is only used for a union
> > discriminator is best named after the union type.
> Yes I agree with your approach too. Will change it to 'MigrationAddressType'
> in the next patchset.
> > > > > +#
> > > > > +# 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
> > > > Migration is between hosts, not "two devices".
> > > Here we are just talking about socket communication right ? So I thought devices might also work.
> > In QEMU, "devices" are the things you create with device_add.
> > 
> > Sockets connect "endpoints".  Also called "peers".
> Ack. 'endpoints' sounds very appropriate to me.
> > > Will change that to 'hosts' as this is in context of migration i.e. MigrattionAddressType
> > > 
> > > > The second sentence confuses me.  What are you trying to say?
> > > I am trying to say that socket is a union in itslef right, so it covers communication transport mechanisms like tcp, unix, vsock and fd already in it.
> > > 
> > > > Also, missing period at the end.
> > > Ack.
> > > 
> > > > > +#
> > > > > +# @exec: Supported communication type to redirect migration stream into file.
> > > > > +#
> > > > > +# @rdma: Supported communication type to redirect rdma type migration stream.
> > > > What about:
> > > > 
> > > >      ##
> > > >      # @MigrationTransport:
> > > >      #
> > > >      # The migration stream transport mechanisms
> > > >      #
> > > >      # @socket: Migrate via socket
> > > >      #
> > > >      # @rdma: Migrate via RDMA
> > > >      #
> > > >      # @file: Direct the migration stream to a file
> > > Should I change from '@exec' to '@file' ?
> > Uh, that change happened somewhere between my conscious thought process
> > and the keyboard ;)
> > 
> > What about
> > 
> >         # @exec: Direct the migration stream to another process
> No worries Markus. Seems okay.
> > > Other than that, it looks better than what I proposed. Will change it.
> > > 
> > > > > +#
> > > > > +# Since 8.1
> > > > > +##
> > > > > +{ 'enum': 'MigrateTransport',
> > > > > +  'data': ['socket', 'exec', 'rdma'] }
> > > > > +
> > > > > +##
> > > > > +# @MigrateExecCommand:
> > > > Documentation of @args is missing.
> > > Ack. Should the naming '@args' be replaced by '@filepath' or @path' or something similar ?
> > Depends on what @args means.
> > 
> > I guess its [program, arg1, arg2, ...].

Yes, that is correct. Essentially this ends up calling

   execve(@args[0], @args)

> > You could split off the program:
> > 
> >      'program: 'str',
> >      'args': [ 'str' ]

Now you also have to declare whether '@args' includes or excludes
'program' as @args[0]. execve() style would be to have '@args[0]'
duplicate 'program'. Personally I think that's overkill for QEMU's
needs. If we don't include 'program' in @args, then we have to
document this, as it isn't discoverable from the QAPI design.

Not separating 'program' and 'args' in QAPI makes it unambiguous
that 'args' must include everything.

> > Try to write clear documentation for both alternatives.  Such an
> > exercise tends to lead me to the one I prefer.
> 
> Hmm, basically here the @args means, for example ['/bin/bash', args1, args2,
> ..., <command>], where command -> /some/file/path.
> 
> Does it even make sense now to break into 3 different parts ?
> 
> 	'program': 'str'
> 	'args': [ 'str' ]
> 	'command': 'str'

Definitely not. This encodes an assumption that we're spawning via a
shell. The intent with the new design is that it lets mgmt apps fully
eliminate use of shell, and directly invoke the program, thus eliminating
potential (security) pitfalls with shell metacharacters.


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