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

[Vladimir Sementsov-Ogievskiy]

On 12.09.25 20:05, Daniel P. Berrangé wrote:

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 <vsement...@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++) {

Ok. Didn't know it's acceptable)

I missed

4b77429adbecf9 "docs/style: permit inline loop variables"

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

Then here we can use:

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

Ok

  }

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

This way, we'll still modify user given fds on failure path (to NULL). But 
that's not a big deal, of course.

      }

return ret;

--
2.48.1

With regards,
Daniel

--
Best regards,
Vladimir