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