[v4,2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

Commit Message

Akihiko Odaki Jan. 15, 2025, 12:03 p.m. UTC

g_spawn_sync() (for GLib < 2.68) or g_spawn_async_with_pipes_and_fds()
(for the newer) gives an informative message if it fails to execute
the script instead of reporting exiting status 1.

g_spawn_check_wait_status() also gives an message easier to understand
than the raw value returned by waitpid().

Signed-off-by: Akihiko Odaki <[e-mail]>
Tested-by: Lei Yang <[e-mail]>
---
 net/tap.c | 220 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 117 insertions(+), 103 deletions(-)

Patch

diff --git a/net/tap.c b/net/tap.c
index ae1c7e398321..f1e00de4181c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -385,56 +385,59 @@  static TAPState *net_tap_fd_init(NetClientState *peer,
     return s;
 }
 
-static void close_all_fds_after_fork(int excluded_fd)
+#if !GLIB_CHECK_VERSION(2, 68, 0)
+static void unset_cloexec(gpointer data)
 {
-    const int skip_fd[] = {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO,
-                           excluded_fd};
-    unsigned int nskip = ARRAY_SIZE(skip_fd);
-
-    /*
-     * skip_fd must be an ordered array of distinct fds, exclude
-     * excluded_fd if already included in the [STDIN_FILENO - STDERR_FILENO]
-     * range
-     */
-    if (excluded_fd <= STDERR_FILENO) {
-        nskip--;
-    }
-
-    qemu_close_all_open_fd(skip_fd, nskip);
+    g_assert(!fcntl(GPOINTER_TO_INT(data), F_SETFD, 0));
 }
+#endif
 
 static void launch_script(const char *setup_script, const char *ifname,
                           int fd, Error **errp)
 {
-    int pid, status;
-    char *args[3];
-    char **parg;
+    int status;
+    const gchar *args[] = { setup_script, ifname, NULL };
+    g_autoptr(GError) error = NULL;
+    bool spawned;
 
     /* try to launch network script */
-    pid = fork();
-    if (pid < 0) {
-        error_setg_errno(errp, errno, "could not launch network script %s",
-                         setup_script);
-        return;
-    }
-    if (pid == 0) {
-        close_all_fds_after_fork(fd);
-        parg = args;
-        *parg++ = (char *)setup_script;
-        *parg++ = (char *)ifname;
-        *parg = NULL;
-        execv(setup_script, args);
-        _exit(1);
-    } else {
+#if GLIB_CHECK_VERSION(2, 68, 0)
+    pid_t pid;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+    spawned = g_spawn_async_with_pipes_and_fds(NULL, args, NULL,
+                                               G_SPAWN_DO_NOT_REAP_CHILD |
+                                               G_SPAWN_CHILD_INHERITS_STDIN,
+                                               NULL, NULL, -1, -1, -1,
+                                               &fd, &fd, 1, &pid,
+                                               NULL, NULL, NULL, &error);
+#pragma GCC diagnostic pop
+    if (spawned) {
         while (waitpid(pid, &status, 0) != pid) {
             /* loop */
         }
+    }
+#else
+    gchar *mutable_args[sizeof(args)];
 
-        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
-            return;
-        }
-        error_setg(errp, "network script %s failed with status %d",
-                   setup_script, status);
+    for (size_t i = 0; i < ARRAY_SIZE(args); i++) {
+        mutable_args[i] = (gchar *)args[i];
+    }
+
+    spawned = g_spawn_sync(NULL, mutable_args, NULL,
+                           G_SPAWN_CHILD_INHERITS_STDIN,
+                           unset_cloexec, GINT_TO_POINTER(fd),
+                           NULL, NULL, &status, &error);
+#endif
+    if (!spawned) {
+        error_setg(errp, "could not launch network script %s: %s",
+                   setup_script, error->message);
+        return;
+    }
+
+    if (!g_spawn_check_wait_status(status, &error)) {
+        error_setg(errp, "network script %s failed: %s",
+                   setup_script, error->message);
     }
 }
 
@@ -477,10 +480,17 @@  static int net_bridge_run_helper(const char *helper, const char *bridge,
 {
     sigset_t oldmask, mask;
     g_autofree char *default_helper = NULL;
+    g_autofree char *fd_buf = NULL;
+    g_autofree char *br_buf = NULL;
+    g_autofree char *helper_cmd = NULL;
+    g_autoptr(GError) error = NULL;
+    int fd;
+    int saved_errno;
     int pid, status;
-    char *args[5];
-    char **parg;
+    const char *args[5];
+    const char **parg;
     int sv[2];
+    bool spawned;
 
     sigemptyset(&mask);
     sigaddset(&mask, SIGCHLD);
@@ -495,82 +505,86 @@  static int net_bridge_run_helper(const char *helper, const char *bridge,
         return -1;
     }
 
-    /* try to launch bridge helper */
-    pid = fork();
-    if (pid < 0) {
-        error_setg_errno(errp, errno, "Can't fork bridge helper");
-        return -1;
-    }
-    if (pid == 0) {
-        char *fd_buf = NULL;
-        char *br_buf = NULL;
-        char *helper_cmd = NULL;
-
-        close_all_fds_after_fork(sv[1]);
-        fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
+    fd_buf = g_strdup_printf("%s%d", "--fd=", sv[1]);
 
-        if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
-            /* assume helper is a command */
+    if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
+        /* assume helper is a command */
 
-            if (strstr(helper, "--br=") == NULL) {
-                br_buf = g_strdup_printf("%s%s", "--br=", bridge);
-            }
-
-            helper_cmd = g_strdup_printf("%s %s %s %s", helper,
-                            "--use-vnet", fd_buf, br_buf ? br_buf : "");
+        if (strstr(helper, "--br=") == NULL) {
+            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
+        }
 
-            parg = args;
-            *parg++ = (char *)"sh";
-            *parg++ = (char *)"-c";
-            *parg++ = helper_cmd;
-            *parg++ = NULL;
+        helper_cmd = g_strdup_printf("%s %s %s %s", helper,
+                        "--use-vnet", fd_buf, br_buf ? br_buf : "");
 
-            execv("/bin/sh", args);
-            g_free(helper_cmd);
-        } else {
-            /* assume helper is just the executable path name */
+        parg = args;
+        *parg++ = "sh";
+        *parg++ = "-c";
+        *parg++ = helper_cmd;
+        *parg++ = NULL;
+    } else {
+        /* assume helper is just the executable path name */
 
-            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
+        br_buf = g_strdup_printf("%s%s", "--br=", bridge);
 
-            parg = args;
-            *parg++ = (char *)helper;
-            *parg++ = (char *)"--use-vnet";
-            *parg++ = fd_buf;
-            *parg++ = br_buf;
-            *parg++ = NULL;
+        parg = args;
+        *parg++ = helper;
+        *parg++ = "--use-vnet";
+        *parg++ = fd_buf;
+        *parg++ = br_buf;
+        *parg++ = NULL;
+    }
 
-            execv(helper, args);
-        }
-        g_free(fd_buf);
-        g_free(br_buf);
-        _exit(1);
+    /* try to launch bridge helper */
+#if GLIB_CHECK_VERSION(2, 68, 0)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+    spawned = g_spawn_async_with_pipes_and_fds(NULL, args, NULL,
+                                               G_SPAWN_DO_NOT_REAP_CHILD |
+                                               G_SPAWN_CHILD_INHERITS_STDIN,
+                                               NULL, NULL, -1, -1, -1,
+                                               &fd, &fd, 1, &pid,
+                                               NULL, NULL, NULL, &error);
+#pragma GCC diagnostic pop
+#else
+    gchar *mutable_args[sizeof(args)];
+
+    for (size_t i = 0; i < ARRAY_SIZE(args); i++) {
+        mutable_args[i] = (gchar *)args[i];
+    }
 
-    } else {
-        int fd;
-        int saved_errno;
+    spawned = g_spawn_async(NULL, mutable_args, NULL,
+                            G_SPAWN_DO_NOT_REAP_CHILD |
+                            G_SPAWN_CHILD_INHERITS_STDIN,
+                            unset_cloexec, GINT_TO_POINTER(sv[1]),
+                            &pid, &error);
+#endif
+    if (!spawned) {
+        error_setg(errp, "could not launch bridge helper: %s", error->message);
+        return -1;
+    }
 
-        close(sv[1]);
+    close(sv[1]);
 
-        fd = RETRY_ON_EINTR(recv_fd(sv[0]));
-        saved_errno = errno;
+    fd = RETRY_ON_EINTR(recv_fd(sv[0]));
+    saved_errno = errno;
 
-        close(sv[0]);
+    close(sv[0]);
 
-        while (waitpid(pid, &status, 0) != pid) {
-            /* loop */
-        }
-        sigprocmask(SIG_SETMASK, &oldmask, NULL);
-        if (fd < 0) {
-            error_setg_errno(errp, saved_errno,
-                             "failed to recv file descriptor");
-            return -1;
-        }
-        if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
-            error_setg(errp, "bridge helper failed");
-            return -1;
-        }
-        return fd;
+    while (waitpid(pid, &status, 0) != pid) {
+        /* loop */
     }
+    sigprocmask(SIG_SETMASK, &oldmask, NULL);
+    if (fd < 0) {
+        error_setg_errno(errp, saved_errno,
+                            "failed to recv file descriptor");
+        return -1;
+    }
+    if (!g_spawn_check_wait_status(status, &error)) {
+        error_setg(errp, "bridge helper failed: %s", error->message);
+        return -1;
+    }
+    return fd;
 }
 
 int net_init_bridge(const Netdev *netdev, const char *name,