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