Re: [PATCH v2 6/6] migration: Deprecate fd: for file migration

[Peter Xu <peterx@redhat.com>]

On Mon, Apr 29, 2024 at 03:47:39PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, Apr 26, 2024 at 10:14:08AM -0300, Fabiano Rosas wrote:
> >> The fd: URI can currently trigger two different types of migration, a
> >> TCP migration using sockets and a file migration using a plain
> >> file. This is in conflict with the recently introduced (8.2) QMP
> >> migrate API that takes structured data as JSON-like format. We cannot
> >> keep the same backend for both types of migration because with the new
> >> API the code is more tightly coupled to the type of transport. This
> >> means a TCP migration must use the 'socket' transport and a file
> >> migration must use the 'file' transport.
> >> 
> >> If we keep allowing fd: when using a file, this creates an issue when
> >> the user converts the old-style (fd:) to the new style ("transport":
> >> "socket") invocation because the file descriptor in question has
> >> previously been allowed to be either a plain file or a socket.
> >> 
> >> To avoid creating too much confusion, we can simply deprecate the fd:
> >> + file usage, which is thought to be rarely used currently and instead
> >> establish a 1:1 correspondence between fd: URI and socket transport,
> >> and file: URI and file transport.
> >> 
> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> ---
> >>  docs/about/deprecated.rst | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >> 
> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> >> index 0fb5c82640..813f7996fe 100644
> >> --- a/docs/about/deprecated.rst
> >> +++ b/docs/about/deprecated.rst
> >> @@ -464,3 +464,17 @@ both, older and future versions of QEMU.
> >>  The ``blacklist`` config file option has been renamed to ``block-rpcs``
> >>  (to be in sync with the renaming of the corresponding command line
> >>  option).
> >> +
> >> +Migration
> >> +---------
> >> +
> >> +``fd:`` URI when used for file migration (since 9.1)
> >> +''''''''''''''''''''''''''''''''''''''''''''''''''''
> >> +
> >> +The ``fd:`` URI can currently provide a file descriptor that
> >> +references either a socket or a plain file. These are two different
> >> +types of migration. In order to reduce ambiguity, the ``fd:`` URI
> >> +usage of providing a file descriptor to a plain file has been
> >> +deprecated in favor of explicitly using the ``file:`` URI with the
> >> +file descriptor being passed as an ``fdset``. Refer to the ``add-fd``
> >> +command documentation for details on the ``fdset`` usage.
> >
> > Wanna do some warn_report() when detected non-socket fds alongside?  Looks
> > like we previously do this for all deprecations.
> 
> Yes, good point.
> 
> >
> > What's the plan when it's support removed?  I'm imaginging that we sanity
> > check fstat() + S_ISSOCK on the fd and fail otherwise?  In that case we can
> > have the code there, dump warn_report(), then switch to failing qmp migrate
> > (and incoming side) later on?
> 
> Something along those lines. We currently use fd_is_socket():
> 
> bool fd_is_socket(int fd)
> {
>     int optval;
>     socklen_t optlen = sizeof(optval);
>     return !getsockopt(fd, SOL_SOCKET, SO_TYPE, &optval, &optlen);
> }
> 
> I'm thinking of this in fd_start_outgoing_migation():
> 
>     if (!fd_is_socket(fd)) {
>         warn_report("fd: migration to a file is deprecated."
>                     " Use file: instead.");
>     }

Sounds good, perhaps also in fd_start_incoming_migration().

-- 
Peter Xu