Re: [PATCH 1/2] chardev: don't set O_NONBLOCK on SCM_RIGHTS file descriptors.

[Stefan Hajnoczi]

On Tue, Jun 14, 2022 at 02:18:42PM +0300, Andrey Ryabinin wrote:
> This reverts commit 9b938c7262e4 ("chardev: clear O_NONBLOCK on SCM_RIGHTS 
> file descriptors").
> File descriptor passed to QEMU via 'getfd' QMP command always
> changed to blocking mode. Instead of that, change blocking mode by QEMU
> file descriptors users when necessary, e.g. like migration.
> 
> We need to preserve the state of the file descriptor in case it's still
> used by an external process and before the QEMU itself started
> using it.
> 
> E.g. our local migration scenario with TAP networking looks like this:
>  1. Create TAP devices and pass file descriptors to source QEMU
>  2. Launch destination QEMU (-incoming defer) and pass same descriptors to it.
>  3. Start migration
> 
> In such scenario setting blocking state at stage (2) will hang source QEMU
> since TAP fd suddenly become blocking.

Is it possible to add a special flag or API for preserving the
O_NONBLOCK open flag? That way the rest of QEMU could continue to safely
reset the flag while the tap fd passing code would explicitly ask for
the O_NONBLOCK open flag to be preserved. That seems safer but I haven't
checked whether it's possible to do this.

> 
> Signed-off-by: Andrey Ryabinin <arbn@yandex-team.com>
> ---
>  chardev/char-socket.c | 3 ---
>  io/channel-socket.c   | 3 ---
>  migration/fd.c        | 2 ++
>  3 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index dc4e218eeb6..c9592fb5836 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -310,9 +310,6 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, 
> size_t len)
>              continue;
>          }
>  
> -        /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> -        qemu_socket_set_block(fd);
> -
>  #ifndef MSG_CMSG_CLOEXEC
>          qemu_set_cloexec(fd);
>  #endif
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index dc9c165de11..8b9679460dc 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -479,9 +479,6 @@ static void qio_channel_socket_copy_fds(struct msghdr 
> *msg,
>                  continue;
>              }
>  
> -            /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> -            qemu_socket_set_block(fd);
> -
>  #ifndef MSG_CMSG_CLOEXEC
>              qemu_set_cloexec(fd);
>  #endif
> diff --git a/migration/fd.c b/migration/fd.c
> index 6f2f50475f4..793fffeb169 100644
> --- a/migration/fd.c
> +++ b/migration/fd.c
> @@ -60,6 +60,8 @@ void fd_start_incoming_migration(const char *fdname, Error 
> **errp)
>          return;
>      }
>  
> +    qemu_socket_set_block(fd);
> +
>      trace_migration_fd_incoming(fd);
>  
>      ioc = qio_channel_new_fd(fd, errp);
> -- 
> 2.35.1
>

signature.asc
Description: PGP signature