On 23/10/2025 09:25, Collin Funk wrote:
I wrote this patch to have 'split' use posix_spawn.While writing it I realized that the setup for posix_spawnattr_t and posix_spawn_file_actions_t takes many lines to check for errors. My assumption is that these functions will almost never fail. If they do, it is probably because of ENOMEM. Not checking for errors would be wrong because, for example, not dup2'ing the pipe file descriptor to become standard input would make the program behave incorrectly. Therefore, I'm considering a xposix-spawnattr and xposix-spawn-file-action Gnulib module. But I haven't decided yet.
I think xposix-spawn... could be overkill as those functions are generally not sprinkled throughout a code base, rather concentrated in certain functions. However we could consolidate the calls to "init" and "setup" groups, which reduces the error() calls and also makes translators job easier. The attached does that, which you can squash into your change if you agree. cheers, Padraig
From bdd457de66f10395a4738b5491f0394b52b3272d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Thu, 23 Oct 2025 13:11:53 +0100 Subject: [PATCH] maint: consolidate posix_spawn functions * src/split.c (create): Group into "init" and "setup" functions, which also consolidates translations. --- src/split.c | 68 +++++++++++++++++++++++------------------------------ 1 file changed, 29 insertions(+), 39 deletions(-) diff --git a/src/split.c b/src/split.c index bd6d2bb56..077e30f0d 100644 --- a/src/split.c +++ b/src/split.c @@ -498,66 +498,56 @@ create (char const *name) } else { - int fd_pair[2]; - pid_t child_pid; if (setenv ("FILE", name, 1) != 0) error (EXIT_FAILURE, errno, _("failed to set FILE environment variable")); if (verbose) fprintf (stdout, _("executing with FILE=%s\n"), quotef (name)); + int result; + int fd_pair[2]; + pid_t child_pid; + posix_spawnattr_t attr; - int result = posix_spawnattr_init (&attr); - if (result != 0) - error (EXIT_FAILURE, result, _("posix_spawnattr_init failed")); - result = posix_spawnattr_setflags (&attr, - (POSIX_SPAWN_USEVFORK - | POSIX_SPAWN_SETSIGDEF)); - if (result != 0) - error (EXIT_FAILURE, result, _("posix_spawnattr_setflags failed")); + posix_spawn_file_actions_t actions; sigset_t set; sigemptyset (&set); if (default_SIGPIPE) sigaddset (&set, SIGPIPE); - result = posix_spawnattr_setsigdefault (&attr, &set); - if (result != 0) - error (EXIT_FAILURE, result, _("posix_spawnattr_setsigdefault failed")); + + if ( (result = posix_spawnattr_init (&attr)) + || (result = posix_spawnattr_setflags (&attr, + (POSIX_SPAWN_USEVFORK + | POSIX_SPAWN_SETSIGDEF))) + || (result = posix_spawnattr_setsigdefault (&attr, &set)) + || (result = posix_spawn_file_actions_init (&actions)) + ) + error (EXIT_FAILURE, result, _("posix_spawn initialization failed")); if (pipe (fd_pair) != 0) error (EXIT_FAILURE, errno, _("failed to create pipe")); - posix_spawn_file_actions_t actions; - result = posix_spawn_file_actions_init (&actions); - if (result != 0) - error (EXIT_FAILURE, result, _("posix_spawn_file_actions_init failed")); /* We have to close any pipes that were opened during an earlier call, otherwise this process will be holding a write-pipe that will prevent the earlier process from reading an EOF on the corresponding read-pipe. */ for (int i = 0; i < n_open_pipes; ++i) - { - result = posix_spawn_file_actions_addclose (&actions, open_pipes[i]); - if (result != 0) - error (EXIT_FAILURE, result, - _("posix_spawn_file_actions_addclose failed")); - } - result = posix_spawn_file_actions_addclose (&actions, fd_pair[1]); - if (result != 0) - error (EXIT_FAILURE, result, - _("posix_spawn_file_actions_addclose failed")); - if (fd_pair[0] != STDIN_FILENO) - { - result = posix_spawn_file_actions_adddup2 (&actions, fd_pair[0], - STDIN_FILENO); - if (result != 0) - error (EXIT_FAILURE, result, - _("posix_spawn_file_actions_adddup2 failed")); - result = posix_spawn_file_actions_addclose (&actions, fd_pair[0]); - if (result != 0) - error (EXIT_FAILURE, result, - _("posix_spawn_file_actions_addclose failed")); - } + if ((result = posix_spawn_file_actions_addclose (&actions, + open_pipes[i]))) + break; + + if ( result + || (result = posix_spawn_file_actions_addclose (&actions, fd_pair[1])) + || (fd_pair[0] != STDIN_FILENO + && ( (result = posix_spawn_file_actions_adddup2 (&actions, + fd_pair[0], + STDIN_FILENO)) + || (result = posix_spawn_file_actions_addclose (&actions, + fd_pair[0])))) + ) + error (EXIT_FAILURE, result, _("posix_spawn setup failed")); + char const *shell_prog = getenv ("SHELL"); if (shell_prog == nullptr) -- 2.51.0
