[PATCH v2 0/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[PATCH v2 0/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Akihiko Odaki]

g_spawn_sync() 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>
---
Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20231219-glib-v1-0-1b040d286b91@daynix.com

---
Akihiko Odaki (2):
      glib-compat: Define g_spawn_check_wait_status()
      tap: Use g_spawn_sync() and g_spawn_check_wait_status()

 include/glib-compat.h |   2 +
 net/tap.c             | 169 ++++++++++++++++++++------------------------------
 2 files changed, 68 insertions(+), 103 deletions(-)
---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20231219-glib-034a34bb05d8

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v2 1/2] glib-compat: Define g_spawn_check_wait_status()

[Akihiko Odaki]

g_spawn_check_exit_status() is renamed to g_spawn_check_wait_status()
in 2.70.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/glib-compat.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index 86be439ba0ef..0bddb327bd5d 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -68,6 +68,8 @@
  * without generating warnings.
  */
 
+#define g_spawn_check_wait_status g_spawn_check_exit_status
+
 /*
  * g_memdup2_qemu:
  * @mem: (nullable): the memory to copy.

-- 
2.47.1

Re: [PATCH v2 1/2] glib-compat: Define g_spawn_check_wait_status()

[Daniel P. Berrangé]

On Sat, Jan 04, 2025 at 05:04:07PM +0900, Akihiko Odaki wrote:
> g_spawn_check_exit_status() is renamed to g_spawn_check_wait_status()
> in 2.70.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/glib-compat.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index 86be439ba0ef..0bddb327bd5d 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -68,6 +68,8 @@
>   * without generating warnings.
>   */
>  
> +#define g_spawn_check_wait_status g_spawn_check_exit_status

This needs to be conditionalized on !GLIB_CHECK_VERSION(2, 70, 0)


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

[PATCH v2 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Akihiko Odaki]

g_spawn_sync() 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>
---
 net/tap.c | 169 ++++++++++++++++++++++++--------------------------------------
 1 file changed, 66 insertions(+), 103 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index ae1c7e398321..392a024f8ed9 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -385,56 +385,30 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
     return s;
 }
 
-static void close_all_fds_after_fork(int excluded_fd)
+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));
 }
 
 static void launch_script(const char *setup_script, const char *ifname,
                           int fd, Error **errp)
 {
-    int pid, status;
-    char *args[3];
-    char **parg;
+    gint status;
+    gchar *argv[] = { (gchar *)setup_script, (gchar *)ifname, NULL };
+    g_autoptr(GError) error = NULL;
 
     /* try to launch network script */
-    pid = fork();
-    if (pid < 0) {
-        error_setg_errno(errp, errno, "could not launch network script %s",
-                         setup_script);
+    if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_CHILD_INHERITS_STDIN,
+                      unset_cloexec, GINT_TO_POINTER(fd),
+                      NULL, NULL, &status, &error)) {
+        error_setg(errp, "could not launch network script %s: %s",
+                   setup_script, error->message);
         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 {
-        while (waitpid(pid, &status, 0) != pid) {
-            /* loop */
-        }
 
-        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
-            return;
-        }
-        error_setg(errp, "network script %s failed with status %d",
-                   setup_script, status);
+    if (!g_spawn_check_wait_status(status, &error)) {
+        error_setg(errp, "network script %s failed: %s",
+                   setup_script, error->message);
     }
 }
 
@@ -477,6 +451,12 @@ 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;
@@ -495,82 +475,65 @@ 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 : "");
-
-            parg = args;
-            *parg++ = (char *)"sh";
-            *parg++ = (char *)"-c";
-            *parg++ = helper_cmd;
-            *parg++ = NULL;
+        if (strstr(helper, "--br=") == NULL) {
+            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
+        }
 
-            execv("/bin/sh", args);
-            g_free(helper_cmd);
-        } else {
-            /* assume helper is just the executable path name */
+        helper_cmd = g_strdup_printf("%s %s %s %s", helper,
+                        "--use-vnet", fd_buf, br_buf ? br_buf : "");
 
-            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
+        parg = args;
+        *parg++ = (char *)"sh";
+        *parg++ = (char *)"-c";
+        *parg++ = helper_cmd;
+        *parg++ = NULL;
+    } else {
+        /* assume helper is just the executable path name */
 
-            parg = args;
-            *parg++ = (char *)helper;
-            *parg++ = (char *)"--use-vnet";
-            *parg++ = fd_buf;
-            *parg++ = br_buf;
-            *parg++ = NULL;
+        br_buf = g_strdup_printf("%s%s", "--br=", bridge);
 
-            execv(helper, args);
-        }
-        g_free(fd_buf);
-        g_free(br_buf);
-        _exit(1);
+        parg = args;
+        *parg++ = (char *)helper;
+        *parg++ = (char *)"--use-vnet";
+        *parg++ = fd_buf;
+        *parg++ = br_buf;
+        *parg++ = NULL;
+    }
 
-    } else {
-        int fd;
-        int saved_errno;
+    /* try to launch bridge helper */
+    if (!g_spawn_async(NULL, args, NULL,
+                       G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_CHILD_INHERITS_STDIN,
+                       unset_cloexec, GINT_TO_POINTER(sv[1]), &pid, &error)) {
+        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

Re: [PATCH v2 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Daniel P. Berrangé]

On Sat, Jan 04, 2025 at 05:04:08PM +0900, Akihiko Odaki wrote:
> g_spawn_sync() 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>
> ---
>  net/tap.c | 169 ++++++++++++++++++++++++--------------------------------------
>  1 file changed, 66 insertions(+), 103 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index ae1c7e398321..392a024f8ed9 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -385,56 +385,30 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>      return s;
>  }
>  
> -static void close_all_fds_after_fork(int excluded_fd)
> +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));
>  }
>  
>  static void launch_script(const char *setup_script, const char *ifname,
>                            int fd, Error **errp)
>  {
> -    int pid, status;
> -    char *args[3];
> -    char **parg;
> +    gint status;
> +    gchar *argv[] = { (gchar *)setup_script, (gchar *)ifname, NULL };
> +    g_autoptr(GError) error = NULL;
>  
>      /* try to launch network script */
> -    pid = fork();
> -    if (pid < 0) {
> -        error_setg_errno(errp, errno, "could not launch network script %s",
> -                         setup_script);
> +    if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_CHILD_INHERITS_STDIN,
> +                      unset_cloexec, GINT_TO_POINTER(fd),
> +                      NULL, NULL, &status, &error)) {

This unset_cloexec callback is relying on knowledge of current internal
impl details of g_spawn_sync. The API docs say that all file descriptors
will be closed, except for stdin/out/err. We should not assume glib is
doing this by setting O_CLOEXEC, as opposed to directly calling close().

If we need specific FDs to remain open, we neeed to be using the other
g_spawn_async_with_pipes_and_fds API that accepts a list of FDs to remain
open.

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

Re: [PATCH v2 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Akihiko Odaki]

On 2025/01/06 18:34, Daniel P. Berrangé wrote:
> On Sat, Jan 04, 2025 at 05:04:08PM +0900, Akihiko Odaki wrote:
>> g_spawn_sync() 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>
>> ---
>>   net/tap.c | 169 ++++++++++++++++++++++++--------------------------------------
>>   1 file changed, 66 insertions(+), 103 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index ae1c7e398321..392a024f8ed9 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -385,56 +385,30 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>>       return s;
>>   }
>>   
>> -static void close_all_fds_after_fork(int excluded_fd)
>> +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));
>>   }
>>   
>>   static void launch_script(const char *setup_script, const char *ifname,
>>                             int fd, Error **errp)
>>   {
>> -    int pid, status;
>> -    char *args[3];
>> -    char **parg;
>> +    gint status;
>> +    gchar *argv[] = { (gchar *)setup_script, (gchar *)ifname, NULL };
>> +    g_autoptr(GError) error = NULL;
>>   
>>       /* try to launch network script */
>> -    pid = fork();
>> -    if (pid < 0) {
>> -        error_setg_errno(errp, errno, "could not launch network script %s",
>> -                         setup_script);
>> +    if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_CHILD_INHERITS_STDIN,
>> +                      unset_cloexec, GINT_TO_POINTER(fd),
>> +                      NULL, NULL, &status, &error)) {
> 
> This unset_cloexec callback is relying on knowledge of current internal
> impl details of g_spawn_sync. The API docs say that all file descriptors
> will be closed, except for stdin/out/err. We should not assume glib is
> doing this by setting O_CLOEXEC, as opposed to directly calling close().
> 
> If we need specific FDs to remain open, we neeed to be using the other
> g_spawn_async_with_pipes_and_fds API that accepts a list of FDs to remain
> open.

g_spawn_async_with_pipes_and_fds() is not available for 2.66 so we 
cannot use it.

An upstream developer says unsetting FD_CLOEXEC in the setup function is 
fine in such a scenario. They are not documenting that because new glib 
versions they are developing has g_spawn_async_with_pipes_and_fds(), 
which is a better alternative:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4452

Regards,
Akihiko Odaki

Re: [PATCH v2 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Daniel P. Berrangé]

On Wed, Jan 15, 2025 at 03:04:19PM +0900, Akihiko Odaki wrote:
> On 2025/01/06 18:34, Daniel P. Berrangé wrote:
> > On Sat, Jan 04, 2025 at 05:04:08PM +0900, Akihiko Odaki wrote:
> > > g_spawn_sync() 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>
> > > ---
> > >   net/tap.c | 169 ++++++++++++++++++++++++--------------------------------------
> > >   1 file changed, 66 insertions(+), 103 deletions(-)
> > > 
> > > diff --git a/net/tap.c b/net/tap.c
> > > index ae1c7e398321..392a024f8ed9 100644
> > > --- a/net/tap.c
> > > +++ b/net/tap.c
> > > @@ -385,56 +385,30 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
> > >       return s;
> > >   }
> > > -static void close_all_fds_after_fork(int excluded_fd)
> > > +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));
> > >   }
> > >   static void launch_script(const char *setup_script, const char *ifname,
> > >                             int fd, Error **errp)
> > >   {
> > > -    int pid, status;
> > > -    char *args[3];
> > > -    char **parg;
> > > +    gint status;
> > > +    gchar *argv[] = { (gchar *)setup_script, (gchar *)ifname, NULL };
> > > +    g_autoptr(GError) error = NULL;
> > >       /* try to launch network script */
> > > -    pid = fork();
> > > -    if (pid < 0) {
> > > -        error_setg_errno(errp, errno, "could not launch network script %s",
> > > -                         setup_script);
> > > +    if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_CHILD_INHERITS_STDIN,
> > > +                      unset_cloexec, GINT_TO_POINTER(fd),
> > > +                      NULL, NULL, &status, &error)) {
> > 
> > This unset_cloexec callback is relying on knowledge of current internal
> > impl details of g_spawn_sync. The API docs say that all file descriptors
> > will be closed, except for stdin/out/err. We should not assume glib is
> > doing this by setting O_CLOEXEC, as opposed to directly calling close().
> > 
> > If we need specific FDs to remain open, we neeed to be using the other
> > g_spawn_async_with_pipes_and_fds API that accepts a list of FDs to remain
> > open.
> 
> g_spawn_async_with_pipes_and_fds() is not available for 2.66 so we cannot
> use it.
> 
> An upstream developer says unsetting FD_CLOEXEC in the setup function is
> fine in such a scenario. They are not documenting that because new glib
> versions they are developing has g_spawn_async_with_pipes_and_fds(), which
> is a better alternative:
> https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4452

That comment only says we're fine to unset FD_CLOEXEC on *historical*
releases of GLib. ie they're not going to break this in a bug fix on
a stable branch of 2.66.x. For anything >= 2.68 they say we need to
be using g_spawn_async_with_pipes_and_fds

IOW, we need to support both code paths today, and in future when we
drop 2.66 compat, we can eliminate the FD_CLOEXEC codepath, leaving
us with only g_spawn_async_with_pipes_and_fds

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

Re: [PATCH v2 2/2] tap: Use g_spawn_sync() and g_spawn_check_wait_status()

[Lei Yang]

I tested this series of patches with virtio-net regression tests,
everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Sat, Jan 4, 2025 at 4:05 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> g_spawn_sync() 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>
> ---
>  net/tap.c | 169 ++++++++++++++++++++++++--------------------------------------
>  1 file changed, 66 insertions(+), 103 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index ae1c7e398321..392a024f8ed9 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -385,56 +385,30 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>      return s;
>  }
>
> -static void close_all_fds_after_fork(int excluded_fd)
> +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));
>  }
>
>  static void launch_script(const char *setup_script, const char *ifname,
>                            int fd, Error **errp)
>  {
> -    int pid, status;
> -    char *args[3];
> -    char **parg;
> +    gint status;
> +    gchar *argv[] = { (gchar *)setup_script, (gchar *)ifname, NULL };
> +    g_autoptr(GError) error = NULL;
>
>      /* try to launch network script */
> -    pid = fork();
> -    if (pid < 0) {
> -        error_setg_errno(errp, errno, "could not launch network script %s",
> -                         setup_script);
> +    if (!g_spawn_sync(NULL, argv, NULL, G_SPAWN_CHILD_INHERITS_STDIN,
> +                      unset_cloexec, GINT_TO_POINTER(fd),
> +                      NULL, NULL, &status, &error)) {
> +        error_setg(errp, "could not launch network script %s: %s",
> +                   setup_script, error->message);
>          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 {
> -        while (waitpid(pid, &status, 0) != pid) {
> -            /* loop */
> -        }
>
> -        if (WIFEXITED(status) && WEXITSTATUS(status) == 0) {
> -            return;
> -        }
> -        error_setg(errp, "network script %s failed with status %d",
> -                   setup_script, status);
> +    if (!g_spawn_check_wait_status(status, &error)) {
> +        error_setg(errp, "network script %s failed: %s",
> +                   setup_script, error->message);
>      }
>  }
>
> @@ -477,6 +451,12 @@ 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;
> @@ -495,82 +475,65 @@ 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 : "");
> -
> -            parg = args;
> -            *parg++ = (char *)"sh";
> -            *parg++ = (char *)"-c";
> -            *parg++ = helper_cmd;
> -            *parg++ = NULL;
> +        if (strstr(helper, "--br=") == NULL) {
> +            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
> +        }
>
> -            execv("/bin/sh", args);
> -            g_free(helper_cmd);
> -        } else {
> -            /* assume helper is just the executable path name */
> +        helper_cmd = g_strdup_printf("%s %s %s %s", helper,
> +                        "--use-vnet", fd_buf, br_buf ? br_buf : "");
>
> -            br_buf = g_strdup_printf("%s%s", "--br=", bridge);
> +        parg = args;
> +        *parg++ = (char *)"sh";
> +        *parg++ = (char *)"-c";
> +        *parg++ = helper_cmd;
> +        *parg++ = NULL;
> +    } else {
> +        /* assume helper is just the executable path name */
>
> -            parg = args;
> -            *parg++ = (char *)helper;
> -            *parg++ = (char *)"--use-vnet";
> -            *parg++ = fd_buf;
> -            *parg++ = br_buf;
> -            *parg++ = NULL;
> +        br_buf = g_strdup_printf("%s%s", "--br=", bridge);
>
> -            execv(helper, args);
> -        }
> -        g_free(fd_buf);
> -        g_free(br_buf);
> -        _exit(1);
> +        parg = args;
> +        *parg++ = (char *)helper;
> +        *parg++ = (char *)"--use-vnet";
> +        *parg++ = fd_buf;
> +        *parg++ = br_buf;
> +        *parg++ = NULL;
> +    }
>
> -    } else {
> -        int fd;
> -        int saved_errno;
> +    /* try to launch bridge helper */
> +    if (!g_spawn_async(NULL, args, NULL,
> +                       G_SPAWN_DO_NOT_REAP_CHILD | G_SPAWN_CHILD_INHERITS_STDIN,
> +                       unset_cloexec, GINT_TO_POINTER(sv[1]), &pid, &error)) {
> +        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
>
>