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.od...@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 :|
