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<[email protected]> writes: > > > > > On 25/05/23 11:04 pm, Markus Armbruster wrote: > > > > Het Gala<[email protected]> 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<[email protected]> > > > > > Signed-off-by: Het Gala<[email protected]> > > > > > Reviewed-by: Juan Quintela<[email protected]> > > > > > Reviewed-by: Daniel P. Berrangé<[email protected]> > > > > > --- > > > > > 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, ...]. > > > > You could split off the program: > > > > 'program: 'str', > > 'args': [ 'str' ] > > > > 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' > > This might probably just need to tewak something in the exec file I guess. > > > > > > + # > > > > > + # Since 8.1 > > > > > + ## > > > > Unwanted indentation. > > > Not able to see any unwanted indentation here ? > > Looks like it got eaten on the way. The last three lines of the doc > > comment are indented: > > > > +## > > +# @MigrateExecCommand: > > + # > > + # Since 8.1 > > + ## > > +{ 'struct': 'MigrateExecCommand', > > + 'data': {'args': [ 'str' ] } } > Yes, you are right. I figured out after replying to you and was looking at > the code. Thanks for noticing it out! Will change spurious indentation in > the v6. > > > > > +{ 'struct': 'MigrateExecCommand', > > > > > + 'data': {'args': [ 'str' ] } } > > > > > + > > > > > +## > > > > > +# @MigrateAddress: > > > > > +# > > > > > +# The options available for communication transport mechanisms for > > > > > migration > > > > Not happy with this sentence (writing good documentation is hard). > > > > > > > > Is the address used for the destination only, or for the source as well? > > > > > > > > If destination only, could it be used for the source at least in theory? > > > > > > > > I'm asking because I need to understand more about intended use to be > > > > able to suggest doc improvements. > > > This address will be used on both destination and source. In code flow, > > > in later patches, changes on destination as well as source have been made > > > to incorporate same definition. > > Does @exec work as source? > > > > Maybe: > > > > # Endpoint address for migration > > > > or > > > > # Migration endpoint configuration > > I think @exec work as source too, because in exec.c file, there are calls > for souce as well as destination. > > I would like to go with "Migration endpoint configuration" because I feel > 'migrate' and 'migrate-incoming' QAPIs are defined in context of live > migration. > > > > > > +# > > > > > +# Since 8.1 > > > > > +## > > > > > +{ 'union': 'MigrateAddress', > > > > > + 'base': { 'transport' : 'MigrateTransport'}, > > > > > + 'discriminator': 'transport', > > > > > + 'data': { > > > > > + 'socket': 'SocketAddress', > > > > > + 'exec': 'MigrateExecCommand', > > > > > + 'rdma': 'InetSocketAddress' } } > > > > > + > > > > 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.
The first four are going to have to be packed back into a SocketAddress struct immediately, as the internal migration APIs all work in terms of a SocketAddress for the inet/unix/vsock/fd case. 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 :|
