* [PATCH v4 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()
2025-01-15 12:03 [PATCH v4 0/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status() Akihiko Odaki
2025-01-15 12:03 ` [PATCH v4 1/2] glib-compat: Define g_spawn_check_wait_status() Akihiko Odaki
@ 2025-01-15 12:03 ` Akihiko Odaki
2025-01-17 13:08 ` Daniel P. Berrangé
1 sibling, 1 reply; 5+ messages in thread
From: Akihiko Odaki @ 2025-01-15 12:03 UTC (permalink / raw)
To: Jason Wang, Daniel P. Berrangé
Cc: qemu-devel, devel, Akihiko Odaki, Lei Yang
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 <akihiko.odaki@daynix.com>
Tested-by: Lei Yang <leiyang@redhat.com>
---
net/tap.c | 220 +++++++++++++++++++++++++++++++++-----------------------------
1 file changed, 117 insertions(+), 103 deletions(-)
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,
--
2.47.1
^ permalink raw reply related [flat|nested] 5+ messages in thread