Re: [PATCH v2 6/8] io/channel-socket: rework qio_channel_socket_copy_fds()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Sep 11, 2025 at 12:20:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We want to switch from qemu_socket_set_block() to newer
> qemu_set_blocking(), which provides return status of operation,
> to handle errors.
> 
> Still, we want to keep qio_channel_socket_readv() interface clean,
> as currently it set @fds and @nfds only on success.
> 
> So, in case of error, we should to close all incoming fds and don't
> touch user's @fds and @nfds.
> 
> Let's make separate functions qio_channel_handle_fds() and
> qio_channel_cleanup_fds(), to achieve what we want.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  io/channel-socket.c | 73 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 16 deletions(-)
> 
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index f7e3cb9742..afae97b2ef 100644
> --- a/io/channel-socket.c
> +++ b/io/channel-socket.c
> @@ -464,8 +464,7 @@ static void qio_channel_socket_finalize(Object *obj)
>  
>  #ifndef WIN32
>  static void qio_channel_socket_copy_fds(struct msghdr *msg,
> -                                        int **fds, size_t *nfds,
> -                                        bool preserve_blocking)
> +                                        int **fds, size_t *nfds)
>  {
>      struct cmsghdr *cmsg;
>  
> @@ -473,7 +472,7 @@ static void qio_channel_socket_copy_fds(struct msghdr *msg,
>      *fds = NULL;
>  
>      for (cmsg = CMSG_FIRSTHDR(msg); cmsg; cmsg = CMSG_NXTHDR(msg, cmsg)) {
> -        int fd_size, i;
> +        int fd_size;
>          int gotfds;
>  
>          if (cmsg->cmsg_len < CMSG_LEN(sizeof(int)) ||
> @@ -491,24 +490,54 @@ static void qio_channel_socket_copy_fds(struct msghdr *msg,
>          gotfds = fd_size / sizeof(int);
>          *fds = g_renew(int, *fds, *nfds + gotfds);
>          memcpy(*fds + *nfds, CMSG_DATA(cmsg), fd_size);
> +        *nfds += gotfds;
> +    }
> +}
>  
> -        for (i = 0; i < gotfds; i++) {
> -            int fd = (*fds)[*nfds + i];
> -            if (fd < 0) {
> -                continue;
> -            }
> +static bool qio_channel_handle_fds(int *fds, size_t nfds,
> +                                   bool preserve_blocking, Error **errp)
> +{
> +    int *end = fds + nfds, *fd;
> +
> +#ifdef MSG_CMSG_CLOEXEC
> +    if (preserve_blocking) {
> +        /* Nothing to do */
> +        return true;
> +    }
> +#endif
>  
> -            if (!preserve_blocking) {
> -                /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> -                qemu_socket_set_block(fd);
> +    for (fd = fds; fd != end; fd++) {
> +        if (*fd < 0) {
> +            continue;
> +        }
> +
> +        if (!preserve_blocking) {
> +            /* O_NONBLOCK is preserved across SCM_RIGHTS so reset it */
> +            if (!qemu_set_blocking(*fd, true, errp)) {
> +                return false;
>              }
> +        }
>  
>  #ifndef MSG_CMSG_CLOEXEC
> -            qemu_set_cloexec(fd);
> +        qemu_set_cloexec(*fd);
>  #endif
> +    }
> +
> +    return true;
> +}
> +
> +static void qio_channel_cleanup_fds(int *fds, size_t nfds)

Suggest we change this to

 ...  qio_channel_cleanup_fds(int **fds, size_t *nfds)

> +{
> +    int *end = fds + nfds, *fd;
> +
> +    for (fd = fds; fd != end; fd++) {

I can't help feeling this would be clearer as

  for (size_t i = 0; i < nfds; i++) {

> +        if (*fd < 0) {
> +            continue;
>          }
> -        *nfds += gotfds;
> +        close(*fd);
>      }
> +
> +    g_free(fds);

Then here we can use:

   g_clear_poointer(fds, g_free);
   *nfds = 0;

>  }
>  
>  
> @@ -559,9 +588,21 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
>      }
>  
>      if (fds && nfds) {
> -        qio_channel_socket_copy_fds(
> -            &msg, fds, nfds,
> -            flags & QIO_CHANNEL_READ_FLAG_FD_PRESERVE_BLOCKING);
> +        int *local_fds;
> +        size_t local_nfds;
> +        bool preserve_blocking =
> +            flags & QIO_CHANNEL_READ_FLAG_FD_PRESERVE_BLOCKING;
> +
> +        qio_channel_socket_copy_fds(&msg, &local_fds, &local_nfds);
> +
> +        if (!qio_channel_handle_fds(local_fds, local_nfds,
> +                                    preserve_blocking, errp)) {
> +            qio_channel_cleanup_fds(local_fds, local_nfds);
> +            return -1;
> +        }
> +
> +        *fds = local_fds;
> +        *nfds = local_nfds;

We could eliminate the 'local_fds' / 'local_nfds' here and directly use
'fds' and 'nfds' when we make qio_channel_cleanup_fds responsible for
clearing the pointers it receives.

>      }
>  
>      return ret;
> -- 
> 2.48.1
> 

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