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

[Fabiano Rosas <farosas@suse.de>]

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.");
    }