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

[Daniel P . Berrangé]

On Fri, Mar 15, 2024 at 09:13:52AM -0300, Fabiano Rosas wrote:
> 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.

Passing plain files to QEMU is a new feature, so apps have to
adapt their code already. The expectations around flags are
not especially hard - RDONLY|WRONLY|RDWR, optionally with
O_DIRECT depending on what migrate feature is enabled.

> 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' } } ]",

Having multiple fdsets should "just work" - qemu_open() handles everything
behind the scenes, so the rest of QEMU shouldn't need to know anything
about FD passing of plain files.

Similarly for socket FD passing, if the QEMU code works with a
SocketAddress struct in its public API, FD passing of of sockets
should again "just work" without any special logic.

The only reason why 'migrate' has this "fd:name" protocol, is because
this pre-dates the introduction of "SocketAddress" in QAPI.

With the switch to "MigrateAddress" exposing "SocketAddress" directly,
'migrate' has normalized itself with best practice.

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

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