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