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

[Markus Armbruster <armbru@redhat.com>]

Het Gala <het.gala@nutanix.com> writes:

> On 05/07/23 4:51 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 | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 45 insertions(+)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 179af0c4d8..e61d25eba2 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1407,6 +1407,51 @@
>>> ##
>>> { 'command': 'migrate-continue', 'data': {'state': 'MigrationStatus'} }
>>> +##
>>> +# @MigrationAddressType:
>>> +#
>>> +# The migration stream transport mechanisms.
>>> +#
>>> +# @socket: Migrate via socket.
>>> +#
>>> +# @exec: Direct the migration stream to another process.
>>> +#
>>> +# @rdma: Migrate via RDMA.
>>> +#
>>> +# Since 8.1
>>> +##
>>> +{ 'enum': 'MigrationAddressType',
>>> +  'data': ['socket', 'exec', 'rdma'] }
>>> +
>>> +##
>>> +# @MigrationExecCommand:
>>> +#
>>> +# @args: list of commands for migraton stream execution to a file.
>>
>> Typo: migration
>>
>>> +#
>>> +# Notes:
>>> +#
>>> +# 1. @args[0] needs to be the path to the new program.
>>
>> @args can't be a "list of commands", as we're spawning just one process.
>> So what is it?
>>
>> Digging through the code with the entire series applied...  Member @args
>> is used in two places:
>>
>> 1. qemu_start_incoming_migration() passes it to
>>     exec_start_incoming_migration(), which translates it into an array
>>     and passes it to qio_channel_command_new_spawn().
>>
>> 2. qmp_migrate() passes it to exec_start_outgoing_migration(), which
>>     does the same.
>>
>> qio_channel_command_new_spawn() passes it to
>> g_spawn_async_with_pipes().  A close read of the latter's documentation
>> leads me to:
>>
>> * args[0] is the excutable's file name.  As usual, a relative name is
>>    relative to the QEMU process's current working directory.
>>
>> * args[1..] are the arguments.
>>
>> Unlike POSIX interfaces like execv() and posix_spawn(), this doesn't
>> separate the executable's file name and 0-th argument.
>>
>> In short, the head of @args is the executable's filename, and the
>> remainder are the arguments.  The fact that the the executable's
>> filename is passed as 0-th argument to the child process is detail.
>>
>> Perhaps this could do:
>>
>>     ##
>>     # @MigrationExecCommand:
>>     #
>>     # @args: command and arguments to execute.
>>
>> If we want more detail, perhaps:
>>
>>     # @args: command (list head) and arguments (list tail) to execute.
>>
>> Not sure we need it.  Thoughts?
>
> From a user prespective, I think defining that command / executable's filename is the list head would be good ? something like
>
> @args: command (list head) and arguments to execute.

Works for me.

[...]