Het Gala <het.g...@nutanix.com> writes:
> On 25/05/23 11:04 pm, Markus Armbruster wrote:
>> Het Gala <het.g...@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.retnaka...@nutanix.com>
>>> Signed-off-by: Het Gala <het.g...@nutanix.com>
>>> Reviewed-by: Juan Quintela <quint...@redhat.com>
>>> Reviewed-by: Daniel P. Berrangé <berra...@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.
>>> +#
>>> +# 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".
> 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
> 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.
>>> + #
>>> + # 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' ] } }
>>> +{ '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
>>> +#
>>> +# 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.
