Re: [RFC PATCH v3 3/3] migration: Add fd to FileMigrationArgs

[Fabiano Rosas]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Mar 15, 2024 at 12:20:40AM -0300, Fabiano Rosas wrote:
>> The fd: URI has supported migration to a file or socket since before
>> QEMU 8.2. In 8.2 we added the file: URI that supported migration to a
>> file. So now we have two ways (three if you count exec:>cat) to
>> migrate to a file. Fine.
>> 
>> However,
>> 
>> In 8.2 we also added the new qmp_migrate API that uses a JSON channel
>> list instead of the URI. It added two migration transports SOCKET and
>> FILE. It was decided that the new API would classify the fd migration
>> as a type of socket migration, neglecting the fact that the fd.c code
>> also supported file migrations.
>> 
>> In 9.0 we're adding support for fd + multifd + mapped-ram, which is
>> tied to the file migration. This was implemented in fd.c, which is
>> only reachable when the SOCKET address type is used.
>> 
>> The result of this is that we're asking users of the new API to create   (1)
>> something called a "socket" to perform migration to a plain file. And
>> creating something called a "file" provides no way of passing in a
>> file descriptor. This is confusing.
>
> The 'file:' protocol eventually calls into qemu_open, and this
> transparently allows for FD passing using /dev/fdset/NNN syntax
> to pass in FDs. 
>

Ok, that's technically correct. But it works differently from
fd:fdname.

/dev/fdset requires the user to switch from getfd to add-fd and it
requires QEMU and libvirt/user to synchronize on the open() flags being
used. QEMU cannot just receive any fd, it must be an fd that matches the
flags QEMU passed into qio_channel_file_new_path().

Users will have to adapt to the new API anyway, so it might be ok to
require these changes around it as well.

To keep compatibility, we'd continue to accept the fd that comes from
"fd:" or SOCKET_ADDRESS_TYPE_FD. But we'd be making it harder for users
of the fd: syntax to move to the new API.

I also don't know how we would deal with fdset when (if) we add support
for passing in more channels in the new API. It supports multiple fds,
but how do we deal with something like:

"[ { 'channel-type': 'main',"
"    'addr': { 'transport': 'file',"
"              'filename': '/dev/fdset/1' } },
"  { 'channel-type': 'second',"
"    'addr': { 'transport': 'file',"
"              'filename': '/dev/fdset/2' } } ]",

Maybe that's too far ahead for this discussion.

>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index aa1b39bce1..37f4b9c6fb 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -1656,13 +1656,20 @@
>>  #
>>  # @filename: The file to receive the migration stream
>>  #
>> +# @fd: A file descriptor name or number.  File descriptors must be
>> +#     first added with the 'getfd' command. (since 9.0).
>> +#
>>  # @offset: The file offset where the migration stream will start
>>  #
>> +# Since 9.0, all members are optional, but at least one of @filename
>> +# or @fd are required.
>> +#
>>  # Since: 8.2
>>  ##
>>  { 'struct': 'FileMigrationArgs',
>> -  'data': { 'filename': 'str',
>> -            'offset': 'uint64' } }
>> +  'data': { '*filename': 'str',
>> +            '*fd': 'str',
>> +            '*offset': 'uint64' } }
>
> Adding 'fd' here is not desirable, because 'filename' is
> resolved via qemu_open which allows for FD passing without
> introducing any new syntax in interfaces which take filenames.
>
> With regards,
> Daniel