Pádraig Brady <[email protected]> writes: >> I notice that glibc's posix_spawn() reaps the child itself upon exec >> error >> (which I confirmed with `strace -ff`) but means we only get a single error >> now from the following: >> # Old behavior >> $ install src/install.c foo -s --strip-program=./foo >> install: cannot run './foo': Permission denied >> install: strip process terminated abnormally >> # New behavior >> $ src/ginstall src/install.c foo -s --strip-program=./foo >> ginstall: cannot run './foo': Permission denied >> That's fine though, and probably a slight improvement.
Yep, that is my preference as well. The second error message didn't seem very helpful to me. Hopefully 'install' isn't running so long that you forget you passed it '--strip-program=./foo'. :) >> I'd add something in NEWS under "Improvements". Perhaps: >> 'install' now uses posix_spawn() to invoke the strip program more >> efficiently. >> We can augment that message for sort(1) and split(1) if we make the >> change there too. >> Otherwise the patch looks good. > > Actually I remembered thinking about not bothering about free'ing > the alloc from file_name_concat() since it was in the temp child process, > but that's no longer the case, so we do need to manage that. Right, good catch. > That also made me realize we should destroy the attr for all errors too. I noticed that issue a little bit after I sent the patch. On glibc and NetBSD it doesn't matter since posix_spawnattr_init is just a memset, and posix_spawnattr_destroy just returns zero. On FreeBSD and OpenBSD posix_spawnattr_init calloc's a structure and posix_spawnattr_destroy just calls free and returns zero. So not calling posix_spawnattr_destroy is an issue there. That also explains why I don't care about the return value of posix_spawnattr_destroy. > The attached should fix both issues and can be squashed into your > patch. Thanks, I pushed the attached patch. Collin
>From 0c1334eb68de303d9ed40a153bb6793bfdcda77c Mon Sep 17 00:00:00 2001 Message-ID: <0c1334eb68de303d9ed40a153bb6793bfdcda77c.1761105097.git.collin.fu...@gmail.com> From: Collin Funk <[email protected]> Date: Sun, 19 Oct 2025 15:22:17 -0700 Subject: [PATCH] install: prefer posix_spawnp to fork and execlp * NEWS: Mention the change. * bootstrap.conf (gnulib_modules): Add posix_spawnattr_destroy, posix_spawnattr_init, posix_spawnattr_setflags, and posix_spawnp. * src/install.c (strip): Use posix_spawnp instead of fork and execlp. --- NEWS | 2 ++ bootstrap.conf | 4 ++++ src/install.c | 56 +++++++++++++++++++++++++++++++++----------------- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/NEWS b/NEWS index 5d70543a1..7641e4f6a 100644 --- a/NEWS +++ b/NEWS @@ -43,6 +43,8 @@ GNU coreutils NEWS -*- outline -*- ** Improvements + 'install' now uses posix_spawn() to invoke the strip program more efficiently. + numfmt now parses numbers with a non-breaking space character before a unit, and parses numbers containing grouping characters from the current locale. It also supports a multi-byte --delimiter character. diff --git a/bootstrap.conf b/bootstrap.conf index 5125d6697..523c4923a 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -213,6 +213,10 @@ gnulib_modules=" pipe-posix pipe2 posix-shell + posix_spawnattr_destroy + posix_spawnattr_init + posix_spawnattr_setflags + posix_spawnp posixtm posixver priv-set diff --git a/src/install.c b/src/install.c index 43c7729af..9cb13d429 100644 --- a/src/install.c +++ b/src/install.c @@ -24,6 +24,7 @@ #include <pwd.h> #include <grp.h> #include <selinux/label.h> +#include <spawn.h> #include <sys/wait.h> #include "system.h" @@ -32,6 +33,7 @@ #include "copy.h" #include "filenamecat.h" #include "full-read.h" +#include "ignore-value.h" #include "mkancesdirs.h" #include "mkdir-p.h" #include "modechange.h" @@ -490,33 +492,49 @@ change_timestamps (struct stat const *src_sb, char const *dest, static bool strip (char const *name) { - int status; - bool ok = false; - pid_t pid = fork (); + posix_spawnattr_t attr; + posix_spawnattr_t *attrp = nullptr; - switch (pid) + /* Try to use vfork for systems where it matters. */ + if (posix_spawnattr_init (&attr) == 0) { - case -1: - error (0, errno, _("fork system call failed")); - break; - case 0: /* Child. */ - { - char const *safe_name = name; - if (name && *name == '-') - safe_name = file_name_concat (".", name, nullptr); - execlp (strip_program, strip_program, safe_name, nullptr); - error (EXIT_FAILURE, errno, _("cannot run %s"), - quoteaf (strip_program)); - } - default: /* Parent. */ + if (posix_spawnattr_setflags (&attr, POSIX_SPAWN_USEVFORK) == 0) + attrp = &attr; + else + ignore_value (posix_spawnattr_destroy (&attr)); + } + + /* Construct the arguments to 'strip'. */ + char *concat_name = nullptr; + char const *safe_name = name; + if (name && *name == '-') + safe_name = concat_name = file_name_concat (".", name, nullptr); + char const *const argv[] = { strip_program, safe_name, nullptr }; + + /* Run 'strip'. */ + pid_t pid; + int result = posix_spawnp (&pid, strip_program, nullptr, attrp, + (char * const *) argv, environ); + + bool ok = false; + if (result != 0) + error (0, result, _("cannot run %s"), quoteaf (strip_program)); + else + { + /* Wait for 'strip' to complete, and emit a warning message on failure */ + int status; if (waitpid (pid, &status, 0) < 0) error (0, errno, _("waiting for strip")); else if (! WIFEXITED (status) || WEXITSTATUS (status)) error (0, 0, _("strip process terminated abnormally")); else - ok = true; /* strip succeeded */ - break; + ok = true; } + + free (concat_name); + if (attrp) + ignore_value (posix_spawnattr_destroy (&attr)); + return ok; } -- 2.51.0
