* [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport.
@ 2017-05-15 19:36 Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-05-15 19:36 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu.
* posix/tst-spawn2.c (do_test): Use libsupport.
* posix/tst-spawn3.c (do_test): Likewise.
---
ChangeLog | 5 ++++
posix/tst-spawn2.c | 26 +++++++++-------
posix/tst-spawn3.c | 88 ++++++++++++++++--------------------------------------
3 files changed, 46 insertions(+), 73 deletions(-)
diff --git a/posix/tst-spawn2.c b/posix/tst-spawn2.c
index 73a37b6..3a2e041 100644
--- a/posix/tst-spawn2.c
+++ b/posix/tst-spawn2.c
@@ -23,11 +23,12 @@
#include <string.h>
#include <unistd.h>
#include <sys/wait.h>
-
#include <stdio.h>
+#include <support/check.h>
+
int
-posix_spawn_test (void)
+do_test (void)
{
/* Check if posix_spawn correctly returns an error and an invalid pid
by trying to spawn an invalid binary. */
@@ -38,35 +39,40 @@ posix_spawn_test (void)
int ret = posix_spawn (&pid, program, 0, 0, args, environ);
if (ret != ENOENT)
- error (EXIT_FAILURE, errno, "posix_spawn");
+ {
+ errno = ret;
+ FAIL_EXIT1 ("posix_spawn: %m");
+ }
/* POSIX states the value returned on pid variable in case of an error
is not specified. GLIBC will update the value iff the child
execution is successful. */
if (pid != -1)
- error (EXIT_FAILURE, errno, "posix_spawn returned pid != -1");
+ FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
/* Check if no child is actually created. */
ret = waitpid (-1, NULL, 0);
if (ret != -1 || errno != ECHILD)
- error (EXIT_FAILURE, errno, "waitpid");
+ FAIL_EXIT1 ("waitpid: %m)");
/* Same as before, but with posix_spawnp. */
char *args2[] = { (char*) program, 0 };
ret = posix_spawnp (&pid, args2[0], 0, 0, args2, environ);
if (ret != ENOENT)
- error (EXIT_FAILURE, errno, "posix_spawnp");
+ {
+ errno = ret;
+ FAIL_EXIT1 ("posix_spawnp: %m");
+ }
if (pid != -1)
- error (EXIT_FAILURE, errno, "posix_spawnp returned pid != -1");
+ FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
ret = waitpid (-1, NULL, 0);
if (ret != -1 || errno != ECHILD)
- error (EXIT_FAILURE, errno, "waitpid");
+ FAIL_EXIT1 ("waitpid: %m)");
return 0;
}
-#define TEST_FUNCTION posix_spawn_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c
index 8577b03..28a4872 100644
--- a/posix/tst-spawn3.c
+++ b/posix/tst-spawn3.c
@@ -25,10 +25,11 @@
#include <unistd.h>
#include <sys/wait.h>
#include <sys/resource.h>
+#include <fcntl.h>
+#include <paths.h>
-static int do_test (void);
-#define TEST_FUNCTION do_test ()
-#include <test-skeleton.c>
+#include <support/check.h>
+#include <support/temp_file.h>
static int
do_test (void)
@@ -47,25 +48,20 @@ do_test (void)
struct rlimit rl;
int max_fd = 24;
+ int ret;
/* Set maximum number of file descriptor to a low value to avoid open
too many files in environments where RLIMIT_NOFILE is large and to
limit the array size to track the opened file descriptors. */
if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
- {
- printf ("error: getrlimit RLIMIT_NOFILE failed");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd);
rl.rlim_cur = max_fd;
if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
- {
- printf ("error: setrlimit RLIMIT_NOFILE to %u failed", max_fd);
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
/* Exhauste the file descriptor limit with temporary files. */
int files[max_fd];
@@ -76,11 +72,7 @@ do_test (void)
if (fd == -1)
{
if (errno != EMFILE)
- {
- printf ("error: create_temp_file returned -1 with "
- "errno != EMFILE\n");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("create_temp_file: %m");
break;
}
files[nfiles++] = fd;
@@ -88,25 +80,16 @@ do_test (void)
posix_spawn_file_actions_t a;
if (posix_spawn_file_actions_init (&a) != 0)
- {
- puts ("error: spawn_file_actions_init failed");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("posix_spawn_file_actions_init");
/* Executes a /bin/sh echo $$ 2>&1 > /tmp/tst-spawn3.pid . */
const char pidfile[] = "/tmp/tst-spawn3.pid";
if (posix_spawn_file_actions_addopen (&a, STDOUT_FILENO, pidfile, O_WRONLY |
O_CREAT | O_TRUNC, 0644) != 0)
- {
- puts ("error: spawn_file_actions_addopen failed");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("posix_spawn_file_actions_addopen");
if (posix_spawn_file_actions_adddup2 (&a, STDOUT_FILENO, STDERR_FILENO) != 0)
- {
- puts ("error: spawn_file_actions_addclose");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("posix_spawn_file_actions_adddup2");
/* Since execve (called by posix_spawn) might require to open files to
actually execute the shell script, setup to close the temporary file
@@ -114,54 +97,40 @@ do_test (void)
for (int i=0; i<nfiles; i++)
{
if (posix_spawn_file_actions_addclose (&a, files[i]))
- {
- printf ("error: posix_spawn_file_actions_addclose failed");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("posix_spawn_file_actions_addclose");
}
char *spawn_argv[] = { (char *) _PATH_BSHELL, (char *) "-c",
(char *) "echo $$", NULL };
pid_t pid;
- if (posix_spawn (&pid, _PATH_BSHELL, &a, NULL, spawn_argv, NULL) != 0)
+ if ((ret = posix_spawn (&pid, _PATH_BSHELL, &a, NULL, spawn_argv, NULL))
+ != 0)
{
- puts ("error: posix_spawn failed");
- exit (EXIT_FAILURE);
+ errno = ret;
+ FAIL_EXIT1 ("posix_spawn: %m");
}
int status;
int err = waitpid (pid, &status, 0);
if (err != pid)
- {
- puts ("error: waitpid failed");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("waitpid: %m");
/* Close the temporary files descriptor so it can check posix_spawn
output. */
for (int i=0; i<nfiles; i++)
{
if (close (files[i]))
- {
- printf ("error: close failed\n");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("close: %m");
}
int pidfd = open (pidfile, O_RDONLY);
if (pidfd == -1)
- {
- printf ("error: open pidfile failed\n");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("open: %m");
char buf[64];
ssize_t n;
if ((n = read (pidfd, buf, sizeof (buf))) < 0)
- {
- printf ("error: read pidfile failed\n");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("read: %m");
unlink (pidfile);
@@ -169,21 +138,14 @@ do_test (void)
char *endp;
long int rpid = strtol (buf, &endp, 10);
if (*endp != '\n')
- {
- printf ("error: didn't parse whole line: \"%s\"\n", buf);
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("*endp != \'n\'");
if (endp == buf)
- {
- puts ("error: read empty line");
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("read empty line");
if (rpid != pid)
- {
- printf ("error: found \"%s\", expected PID %ld\n", buf, (long int) pid);
- exit (EXIT_FAILURE);
- }
+ FAIL_EXIT1 ("found \"%s\", expected pid %ld\n", buf, (long int) pid);
return 0;
}
+
+#include <support/test-driver.c>
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-05-15 19:36 [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
@ 2017-05-15 19:36 ` Adhemerval Zanella
2017-06-28 20:27 ` Adhemerval Zanella
2017-06-29 14:22 ` Andreas Schwab
2017-05-15 19:36 ` [PATCH 3/3] posix: Refactor posix_spawn Adhemerval Zanella
2017-06-28 12:47 ` [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
2 siblings, 2 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-05-15 19:36 UTC (permalink / raw)
To: libc-alpha
This patch improves the default posix implementation of posix_spawn{p}
and align with Linux one. The main idea is to try shared common
implementation bits with Linux and avoid code duplication, fix some
issues already fixed in Linux code, and deprecated vfork internal
usage (source of various bug reports). In a short:
- It moves POSIX_SPAWN_USEVFORK usage and sets it a no-op. Since
the process that actually spawn the new process do not share
memory with parent (with vfork), it fixes BZ#14750 for this
implementation.
- It uses a pipe to correctly obtain the return upon failure
of execution (BZ#18433).
- It correctly enable/disable asynchronous cancellation (checked
on ptl/tst-exec5.c).
- It correctly disable/enable signal handling.
Using this version instead of Linux shows only one regression,
posix/tst-spawn3, because of pipe2 usage which increase total
number of file descriptor.
* sysdeps/posix/spawni.c (__spawni_child): New function.
(__spawni): Rename to __spawnix.
---
ChangeLog | 3 +
sysdeps/posix/spawni.c | 362 ++++++++++++++++++++++++-------------------------
2 files changed, 182 insertions(+), 183 deletions(-)
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index 9cad25c..e096a42 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -16,20 +16,23 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <errno.h>
+#include <spawn.h>
+#include <assert.h>
#include <fcntl.h>
#include <paths.h>
-#include <spawn.h>
-#include <stdbool.h>
-#include <stdlib.h>
#include <string.h>
-#include <unistd.h>
-#include <signal.h>
#include <sys/resource.h>
-#include "spawn_int.h"
+#include <sys/wait.h>
+#include <sys/param.h>
+#include <sys/mman.h>
#include <not-cancel.h>
#include <local-setxid.h>
#include <shlib-compat.h>
+#include <nptl/pthreadP.h>
+#include <dl-sysdep.h>
+#include <libc-pointer-arith.h>
+#include <ldsodefs.h>
+#include "spawn_int.h"
/* The Unix standard contains a long explanation of the way to signal
@@ -39,94 +42,59 @@
normal program exit with the exit code 127. */
#define SPAWN_ERROR 127
-
-/* The file is accessible but it is not an executable file. Invoke
- the shell to interpret it as a script. */
-static void
-internal_function
-script_execute (const char *file, char *const argv[], char *const envp[])
+struct posix_spawn_args
{
- /* Count the arguments. */
- int argc = 0;
- while (argv[argc++])
- ;
-
- /* Construct an argument list for the shell. */
- {
- char *new_argv[argc + 1];
- new_argv[0] = (char *) _PATH_BSHELL;
- new_argv[1] = (char *) file;
- while (argc > 1)
- {
- new_argv[argc] = argv[argc - 1];
- --argc;
- }
-
- /* Execute the shell. */
- __execve (new_argv[0], new_argv, envp);
- }
-}
-
-static inline void
-maybe_script_execute (const char *file, char *const argv[], char *const envp[],
- int xflags)
+ sigset_t oldmask;
+ const char *file;
+ int (*exec) (const char *, char *const *, char *const *);
+ const posix_spawn_file_actions_t *fa;
+ const posix_spawnattr_t *restrict attr;
+ char *const *argv;
+ ptrdiff_t argc;
+ char *const *envp;
+ int xflags;
+ int pipe[2];
+};
+
+/* Older version requires that shell script without shebang definition
+ to be called explicitly using /bin/sh (_PATH_BSHELL). */
+static void
+maybe_script_execute (struct posix_spawn_args *args)
{
if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
- && (xflags & SPAWN_XFLAGS_TRY_SHELL)
- && errno == ENOEXEC)
- script_execute (file, argv, envp);
-}
-
-/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
- Before running the process perform the actions described in FILE-ACTIONS. */
-int
-__spawni (pid_t *pid, const char *file,
- const posix_spawn_file_actions_t *file_actions,
- const posix_spawnattr_t *attrp, char *const argv[],
- char *const envp[], int xflags)
-{
- pid_t new_pid;
- char *path, *p, *name;
- size_t len;
- size_t pathlen;
-
- /* Do this once. */
- short int flags = attrp == NULL ? 0 : attrp->__flags;
-
- /* Generate the new process. */
- if ((flags & POSIX_SPAWN_USEVFORK) != 0
- /* If no major work is done, allow using vfork. Note that we
- might perform the path searching. But this would be done by
- a call to execvp(), too, and such a call must be OK according
- to POSIX. */
- || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
- | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
- | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS
- | POSIX_SPAWN_SETSID)) == 0
- && file_actions == NULL))
- new_pid = __vfork ();
- else
- new_pid = __fork ();
-
- if (new_pid != 0)
+ && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC)
{
- if (new_pid < 0)
- return errno;
-
- /* The call was successful. Store the PID if necessary. */
- if (pid != NULL)
- *pid = new_pid;
+ char *const *argv = args->argv;
+ ptrdiff_t argc = args->argc;
+
+ /* Construct an argument list for the shell. */
+ char *new_argv[argc + 1];
+ new_argv[0] = (char *) _PATH_BSHELL;
+ new_argv[1] = (char *) args->file;
+ if (argc > 1)
+ memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
+ else
+ new_argv[2] = NULL;
- return 0;
+ /* Execute the shell. */
+ args->exec (new_argv[0], new_argv, args->envp);
}
+}
+
+/* Function used in the clone call to setup the signals mask, posix_spawn
+ attributes, and file actions. */
+static int
+__spawni_child (void *arguments)
+{
+ struct posix_spawn_args *args = arguments;
+ const posix_spawnattr_t *restrict attr = args->attr;
+ const posix_spawn_file_actions_t *file_actions = args->fa;
+ int ret;
- /* Set signal mask. */
- if ((flags & POSIX_SPAWN_SETSIGMASK) != 0
- && __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0)
- _exit (SPAWN_ERROR);
+ __close (args->pipe[0]);
/* Set signal default action. */
- if ((flags & POSIX_SPAWN_SETSIGDEF) != 0)
+ if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0)
{
/* We have to iterate over all signals. This could possibly be
done better but it requires system specific solutions since
@@ -139,41 +107,41 @@ __spawni (pid_t *pid, const char *file,
sa.sa_handler = SIG_DFL;
for (sig = 1; sig <= _NSIG; ++sig)
- if (__sigismember (&attrp->__sd, sig) != 0
+ if (__sigismember (&attr->__sd, sig) != 0
&& __sigaction (sig, &sa, NULL) != 0)
- _exit (SPAWN_ERROR);
-
+ goto fail;
}
#ifdef _POSIX_PRIORITY_SCHEDULING
/* Set the scheduling algorithm and parameters. */
- if ((flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
+ if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
== POSIX_SPAWN_SETSCHEDPARAM)
{
- if (__sched_setparam (0, &attrp->__sp) == -1)
- _exit (SPAWN_ERROR);
+ if ((ret = __sched_setparam (0, &attr->__sp)) == -1)
+ goto fail;
}
- else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0)
+ else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0)
{
- if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1)
- _exit (SPAWN_ERROR);
+ if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1))
+ goto fail;
}
#endif
- if ((flags & POSIX_SPAWN_SETSID) != 0
- && __setsid () < 0)
- _exit (SPAWN_ERROR);
+ /* Set the process session ID. */
+ if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
+ && (ret = __setsid ()) < 0)
+ goto fail;
/* Set the process group ID. */
- if ((flags & POSIX_SPAWN_SETPGROUP) != 0
- && __setpgid (0, attrp->__pgrp) != 0)
- _exit (SPAWN_ERROR);
+ if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
+ && (ret =__setpgid (0, attr->__pgrp)) != 0)
+ goto fail;
/* Set the effective user and group IDs. */
- if ((flags & POSIX_SPAWN_RESETIDS) != 0
- && (local_seteuid (__getuid ()) != 0
- || local_setegid (__getgid ()) != 0))
- _exit (SPAWN_ERROR);
+ if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0
+ && ((ret = local_seteuid (__getuid ())) != 0
+ || (ret = local_setegid (__getgid ())) != 0))
+ goto fail;
/* Execute the file actions. */
if (file_actions != NULL)
@@ -191,7 +159,7 @@ __spawni (pid_t *pid, const char *file,
case spawn_do_close:
if (close_not_cancel (action->action.close_action.fd) != 0)
{
- if (! have_fdlimit)
+ if (have_fdlimit == 0)
{
__getrlimit64 (RLIMIT_NOFILE, &fdlimit);
have_fdlimit = true;
@@ -200,120 +168,148 @@ __spawni (pid_t *pid, const char *file,
/* Only signal errors for file descriptors out of range. */
if (action->action.close_action.fd < 0
|| action->action.close_action.fd >= fdlimit.rlim_cur)
- /* Signal the error. */
- _exit (SPAWN_ERROR);
+ {
+ ret = -1;
+ goto fail;
+ }
}
break;
case spawn_do_open:
{
+ /* POSIX states that if fildes was already an open file descriptor,
+ it shall be closed before the new file is opened. This avoid
+ pontential issues when posix_spawn plus addopen action is called
+ with the process already at maximum number of file descriptor
+ opened and also for multiple actions on single-open special
+ paths (like /dev/watchdog). */
+ close_not_cancel (action->action.open_action.fd);
+
int new_fd = open_not_cancel (action->action.open_action.path,
action->action.open_action.oflag
| O_LARGEFILE,
action->action.open_action.mode);
- if (new_fd == -1)
- /* The `open' call failed. */
- _exit (SPAWN_ERROR);
+ if ((ret = new_fd) == -1)
+ goto fail;
/* Make sure the desired file descriptor is used. */
if (new_fd != action->action.open_action.fd)
{
- if (__dup2 (new_fd, action->action.open_action.fd)
+ if ((ret = __dup2 (new_fd, action->action.open_action.fd))
!= action->action.open_action.fd)
- /* The `dup2' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
- if (close_not_cancel (new_fd) != 0)
- /* The `close' call failed. */
- _exit (SPAWN_ERROR);
+ if ((ret = close_not_cancel (new_fd) != 0))
+ goto fail;
}
}
break;
case spawn_do_dup2:
- if (__dup2 (action->action.dup2_action.fd,
- action->action.dup2_action.newfd)
+ if ((ret = __dup2 (action->action.dup2_action.fd,
+ action->action.dup2_action.newfd))
!= action->action.dup2_action.newfd)
- /* The `dup2' call failed. */
- _exit (SPAWN_ERROR);
+ goto fail;
break;
}
}
}
- if ((xflags & SPAWN_XFLAGS_USE_PATH) == 0 || strchr (file, '/') != NULL)
- {
- /* The FILE parameter is actually a path. */
- __execve (file, argv, envp);
+ /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
+ is set, otherwise restore the previous one. */
+ __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
+ ? &attr->__ss : &args->oldmask, 0);
- maybe_script_execute (file, argv, envp, xflags);
+ args->exec (args->file, args->argv, args->envp);
- /* Oh, oh. `execve' returns. This is bad. */
- _exit (SPAWN_ERROR);
- }
+ /* This is compatibility function required to enable posix_spawn run
+ script without shebang definition for older posix_spawn versions
+ (2.15). */
+ maybe_script_execute (args);
- /* We have to search for FILE on the path. */
- path = getenv ("PATH");
- if (path == NULL)
- {
- /* There is no `PATH' in the environment.
- The default search path is the current directory
- followed by the path `confstr' returns for `_CS_PATH'. */
- len = confstr (_CS_PATH, (char *) NULL, 0);
- path = (char *) __alloca (1 + len);
- path[0] = ':';
- (void) confstr (_CS_PATH, path + 1, len);
- }
+ ret = -errno;
- len = strlen (file) + 1;
- pathlen = strlen (path);
- name = __alloca (pathlen + len + 1);
- /* Copy the file name at the top. */
- name = (char *) memcpy (name + pathlen + 1, file, len);
- /* And add the slash. */
- *--name = '/';
+fail:
+ /* Since sizeof errno < PIPE_BUF, the write is atomic. */
+ ret = -ret;
+ if (ret)
+ while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0);
- p = path;
- do
- {
- char *startp;
+ _exit (SPAWN_ERROR);
+}
- path = p;
- p = __strchrnul (path, ':');
+/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
+ Before running the process perform the actions described in FILE-ACTIONS. */
+int
+__spawnix (pid_t *pid, const char *file,
+ const posix_spawn_file_actions_t *file_actions,
+ const posix_spawnattr_t *attrp, char *const argv[],
+ char *const envp[], int xflags,
+ int (*exec) (const char *, char *const *, char *const *))
+{
+ struct posix_spawn_args args;
+ int ec;
- if (p == path)
- /* Two adjacent colons, or a colon at the beginning or the end
- of `PATH' means to search the current directory. */
- startp = name + 1;
- else
- startp = (char *) memcpy (name - (p - path), path, p - path);
+ if (__pipe2 (args.pipe, O_CLOEXEC))
+ return errno;
- /* Try to execute this name. If it works, execv will not return. */
- __execve (startp, argv, envp);
+ /* Disable asynchronous cancellation. */
+ int state;
+ __libc_ptf_call (__pthread_setcancelstate,
+ (PTHREAD_CANCEL_DISABLE, &state), 0);
- maybe_script_execute (startp, argv, envp, xflags);
+ ptrdiff_t argc = 0;
+ ptrdiff_t limit = INT_MAX - 1;
+ while (argv[argc++] != NULL)
+ if (argc == limit)
+ {
+ errno = E2BIG;
+ return errno;
+ }
- switch (errno)
- {
- case EACCES:
- case ENOENT:
- case ESTALE:
- case ENOTDIR:
- /* Those errors indicate the file is missing or not executable
- by us, in which case we want to just try the next path
- directory. */
- break;
-
- default:
- /* Some other error means we found an executable file, but
- something went wrong executing it; return the error to our
- caller. */
- _exit (SPAWN_ERROR);
- }
+ args.file = file;
+ args.exec = exec;
+ args.fa = file_actions;
+ args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 };
+ args.argv = argv;
+ args.argc = argc;
+ args.envp = envp;
+ args.xflags = xflags;
+
+ /* Generate the new process. */
+ pid_t new_pid = __fork ();
+
+ if (new_pid == 0)
+ __spawni_child (&args);
+ else if (new_pid > 0)
+ {
+ __close (args.pipe[1]);
+
+ if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
+ ec = 0;
+ else
+ __waitpid (new_pid, &(int) { 0 }, 0);
}
- while (*p++ != '\0');
+ else
+ ec = -new_pid;
- /* Return with an error. */
- _exit (SPAWN_ERROR);
+ __close (args.pipe[0]);
+
+ if ((ec == 0) && (pid != NULL))
+ *pid = new_pid;
+
+ __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
+
+ return ec;
+}
+
+int
+__spawni (pid_t * pid, const char *file,
+ const posix_spawn_file_actions_t * acts,
+ const posix_spawnattr_t * attrp, char *const argv[],
+ char *const envp[], int xflags)
+{
+ return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
+ xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
}
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] posix: Refactor posix_spawn
2017-05-15 19:36 [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
@ 2017-05-15 19:36 ` Adhemerval Zanella
2017-06-28 1:07 ` Rasmus Villemoes
2017-06-28 12:47 ` [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
2 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2017-05-15 19:36 UTC (permalink / raw)
To: libc-alpha
This patch refactor both POSIX and Linux internal posix_spawn to use
a common implementation. The POSIX one is parametrized to use define
hooks and Linux reimplements it on its file. No functional change is
expected.
Checked on x86_64-linux-gnu.
* sysdeps/posix/spawni.c (_POSIX_SPAWN_SYNC_ARGS): New define.
(_POSIX_SPAWN_CHILD_SETUP_SIGNAL): Likewise.
(_POSIX_SPAWN_CHILD_SYNC): Likewise.
(_POSIX_SPAWN_CHILD_SYNC_END): Likewise.
(_POSIX_SPAWN_PARENT_SYNC_START): Likewise.
(_POSIX_SPAWN_PARENT_PROCESS): Likewise.
(_POSIX_SPAWN_PARENT_BLOCK_SIGNALS): Likewise.
(_POSIX_SPAWN_PARENT_RESTORE_SIGNALS): Likewise.
(__spawni_child_setup_signals): New function.
(__spawn_process): Likewise.
(struct posix_spawn_args): Use _POSIX_SPAWN_SYNC_ARGS.
(__spawni_child): Use _POSIX_SPAWN_CHILD_SYNC,
_POSIX_SPAWN_CHILD_SETUP_SIGNAL, and
_POSIX_SPAWN_CHILD_SYNC_PRE_EXEC.
(__spawnix): Use _POSIX_SPAWN_PARENT_SYNC_START,
_POSIX_SPAWN_PARENT_BLOCK_SIGNALS, _POSIX_SPAWN_PARENT_PROCESS,
and _POSIX_SPAWN_PARENT_RESTORE_SIGNALS.
* sysdeps/unix/sysv/linux/spawni.c (_POSIX_SPAWN_SYNC_ARGS): Define.
(_POSIX_SPAWN_CHILD_SYNC): Likewise.
(_POSIX_SPAWN_CHILD_SETUP_SIGNAL): Likewise.
(_POSIX_SPAWN_CHILD_SYNC_PRE_EXEC): Likewise.
(_POSIX_SPAWN_CHILD_SYNC_END): Likewise.
(_POSIX_SPAWN_PARENT_SYNC_START: Likewise.
(_POSIX_SPAWN_PARENT_PROCESS): Likewise.
(_POSIX_SPAWN_PARENT_BLOCK_SIGNALS): Likewise.
(_POSIX_SPAWN_PARENT_RESTORE_SIGNALS): Likewise.
(__spawni_child_setup_signals): New function.
(__spawn_process): Likewise.
(maybe_script_execute): Remove function.
(struct posix_spawn_args): Remove definition.
---
ChangeLog | 31 ++++
sysdeps/posix/spawni.c | 190 ++++++++++++++++------
sysdeps/unix/sysv/linux/spawni.c | 333 +++++++--------------------------------
3 files changed, 233 insertions(+), 321 deletions(-)
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index e096a42..35130d3 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -1,4 +1,4 @@
-/* Guts of POSIX spawn interface. Generic POSIX.1 version.
+/* Generic POSIXX.1 posix_spawn implementation.
Copyright (C) 2000-2017 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -42,6 +42,12 @@
normal program exit with the exit code 127. */
#define SPAWN_ERROR 127
+/* Additional variable to synchronize the error value with parent. For
+ default implementation it is pipe. */
+#ifndef _POSIX_SPAWN_SYNC_ARGS
+# define _POSIX_SPAWN_SYNC_ARGS int pipe[2]
+#endif
+
struct posix_spawn_args
{
sigset_t oldmask;
@@ -53,9 +59,120 @@ struct posix_spawn_args
ptrdiff_t argc;
char *const *envp;
int xflags;
- int pipe[2];
+ _POSIX_SPAWN_SYNC_ARGS;
};
+/* Any steps required to setup the signal disposition for
+ POSIX_SPAWN_SETSIGDEF flag. */
+#ifndef _POSIX_SPAWN_CHILD_SETUP_SIGNAL
+static void
+__spawni_child_setup_signals (const posix_spawnattr_t *restrict attr)
+{
+ /* Set signal default action. */
+ if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0)
+ {
+ /* We have to iterate over all signals. This could possibly be
+ done better but it requires system specific solutions since
+ the sigset_t data type can be very different on different
+ architectures. */
+ int sig;
+ struct sigaction sa;
+
+ memset (&sa, '\0', sizeof (sa));
+ sa.sa_handler = SIG_DFL;
+
+ for (sig = 1; sig <= _NSIG; ++sig)
+ if (__sigismember (&attr->__sd, sig) != 0)
+ /* Ignore erros for invalid signals/handlers. */
+ __sigaction (sig, &sa, NULL);
+ }
+}
+# define _POSIX_SPAWN_CHILD_SETUP_SIGNAL(__attr) \
+ __spawni_child_setup_signals (__attr)
+#endif
+
+/* Any steps required to handle the error sync in the child. */
+#ifndef _POSIX_SPAWN_CHILD_SYNC
+# define _POSIX_SPAWN_CHILD_SYNC(__args) \
+ __close (__args->pipe[0])
+#endif
+
+/* Any steps required to handle the error sync just before the call to
+ execve/execvep. */
+#ifndef _POSIX_SPAWN_CHILD_SYNC_PRE_EXEC
+# define _POSIX_SPAWN_CHILD_SYNC_PRE_EXEC(__args)
+#endif
+
+/* Any steps required to handle the error sync after a failed execve/execvep
+ call. */
+#ifndef _POSIX_SPAWN_CHILD_SYNC_END
+/* Write down the error on the pipe. Since sizeof errno < PIPE_BUF, the write
+ is atomic. */
+# define _POSIX_SPAWN_CHILD_SYNC_END(__ret, __args) \
+ ({ \
+ int __err = -ret; \
+ if (__err) \
+ while (write_not_cancel (args->pipe[1], &__err, sizeof (__err)) < 0); \
+ })
+#endif
+
+/* Any steps required to handle the erro sync in parent before child
+ creation. */
+#ifndef _POSIX_SPAWN_PARENT_SYNC_START
+# define _POSIX_SPAWN_PARENT_SYNC_START(__args) \
+ if (__pipe2 (__args.pipe, O_CLOEXEC)) \
+ return errno;
+#endif
+
+static int __spawni_child (void *arguments);
+
+/* The routine to create the spawn process and synchronize the error handling
+ with parent. */
+#ifndef _POSIX_SPAWN_PARENT_PROCESS
+/* For default implementation, create the spawn process using fork plus
+ execve and send any possible creation error using the pipe. */
+static int
+__spawn_process (pid_t *pid, struct posix_spawn_args *args)
+{
+ int ec;
+
+ pid_t new_pid = __fork ();
+
+ if (new_pid == 0)
+ __spawni_child (args);
+ else if (new_pid > 0)
+ {
+ __close (args->pipe[1]);
+
+ if (__read (args->pipe[0], &ec, sizeof ec) != sizeof ec)
+ ec = 0;
+ else
+ __waitpid (new_pid, &(int) { 0 }, 0);
+ }
+ else
+ ec = -new_pid;
+
+ close (args->pipe[0]);
+
+ *pid = new_pid;
+ return ec;
+}
+# define _POSIX_SPAWN_PARENT_PROCESS(__pid, __argc, __args) \
+ __spawn_process (__pid, __args)
+#endif
+
+/* Block the signal before process spawn. */
+#ifndef _POSIX_SPAWN_PARENT_BLOCK_SIGNALS
+# define _POSIX_SPAWN_PARENT_BLOCK_SIGNALS(__args) \
+ __sigprocmask (SIG_BLOCK, &SIGALL_SET, &__args.oldmask)
+#endif
+
+/* Restore the signal after process spawn. */
+#ifndef _POSIX_SPAWN_PARENT_RESTORE_SIGNALS
+# define _POSIX_SPAWN_PARENT_RESTORE_SIGNALS(__args) \
+ __sigprocmask (SIG_SETMASK, &__args.oldmask, 0)
+#endif
+
/* Older version requires that shell script without shebang definition
to be called explicitly using /bin/sh (_PATH_BSHELL). */
static void
@@ -91,26 +208,9 @@ __spawni_child (void *arguments)
const posix_spawn_file_actions_t *file_actions = args->fa;
int ret;
- __close (args->pipe[0]);
-
- /* Set signal default action. */
- if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0)
- {
- /* We have to iterate over all signals. This could possibly be
- done better but it requires system specific solutions since
- the sigset_t data type can be very different on different
- architectures. */
- int sig;
- struct sigaction sa;
-
- memset (&sa, '\0', sizeof (sa));
- sa.sa_handler = SIG_DFL;
+ _POSIX_SPAWN_CHILD_SYNC (args);
- for (sig = 1; sig <= _NSIG; ++sig)
- if (__sigismember (&attr->__sd, sig) != 0
- && __sigaction (sig, &sa, NULL) != 0)
- goto fail;
- }
+ _POSIX_SPAWN_CHILD_SETUP_SIGNAL (attr);
#ifdef _POSIX_PRIORITY_SCHEDULING
/* Set the scheduling algorithm and parameters. */
@@ -221,6 +321,7 @@ __spawni_child (void *arguments)
__sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
? &attr->__ss : &args->oldmask, 0);
+ _POSIX_SPAWN_CHILD_SYNC_PRE_EXEC (args);
args->exec (args->file, args->argv, args->envp);
/* This is compatibility function required to enable posix_spawn run
@@ -231,10 +332,7 @@ __spawni_child (void *arguments)
ret = -errno;
fail:
- /* Since sizeof errno < PIPE_BUF, the write is atomic. */
- ret = -ret;
- if (ret)
- while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0);
+ _POSIX_SPAWN_CHILD_SYNC_END (ret, args);
_exit (SPAWN_ERROR);
}
@@ -251,15 +349,17 @@ __spawnix (pid_t *pid, const char *file,
struct posix_spawn_args args;
int ec;
- if (__pipe2 (args.pipe, O_CLOEXEC))
- return errno;
-
- /* Disable asynchronous cancellation. */
- int state;
- __libc_ptf_call (__pthread_setcancelstate,
- (PTHREAD_CANCEL_DISABLE, &state), 0);
+ _POSIX_SPAWN_PARENT_SYNC_START (args);
+ /* To avoid imposing hard limits on posix_spawn{p} the total number of
+ arguments is first calculated to allocate a mmap to hold all possible
+ values. */
ptrdiff_t argc = 0;
+ /* Linux allows at most max (0x7FFFFFFF, 1/4 stack size) arguments
+ to be used in a execve call. We limit to INT_MAX minus one due the
+ compatiblity code that may execute a shell script (maybe_script_execute)
+ where it will construct another argument list with an additional
+ argument. */
ptrdiff_t limit = INT_MAX - 1;
while (argv[argc++] != NULL)
if (argc == limit)
@@ -268,6 +368,11 @@ __spawnix (pid_t *pid, const char *file,
return errno;
}
+ /* Disable asynchronous cancellation. */
+ int state;
+ __libc_ptf_call (__pthread_setcancelstate,
+ (PTHREAD_CANCEL_DISABLE, &state), 0);
+
args.file = file;
args.exec = exec;
args.fa = file_actions;
@@ -277,28 +382,17 @@ __spawnix (pid_t *pid, const char *file,
args.envp = envp;
args.xflags = xflags;
- /* Generate the new process. */
- pid_t new_pid = __fork ();
-
- if (new_pid == 0)
- __spawni_child (&args);
- else if (new_pid > 0)
- {
- __close (args.pipe[1]);
+ _POSIX_SPAWN_PARENT_BLOCK_SIGNALS (args);
- if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
- ec = 0;
- else
- __waitpid (new_pid, &(int) { 0 }, 0);
- }
- else
- ec = -new_pid;
+ pid_t new_pid = 0;
- __close (args.pipe[0]);
+ ec = _POSIX_SPAWN_PARENT_PROCESS (&new_pid, argc, &args);
if ((ec == 0) && (pid != NULL))
*pid = new_pid;
+ _POSIX_SPAWN_PARENT_RESTORE_SIGNALS (args);
+
__libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
return ec;
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index c56f894..2588046 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -17,22 +17,8 @@
<http://www.gnu.org/licenses/>. */
#include <spawn.h>
-#include <assert.h>
-#include <fcntl.h>
-#include <paths.h>
-#include <string.h>
-#include <sys/resource.h>
-#include <sys/wait.h>
-#include <sys/param.h>
-#include <sys/mman.h>
-#include <not-cancel.h>
-#include <local-setxid.h>
-#include <shlib-compat.h>
-#include <nptl/pthreadP.h>
-#include <dl-sysdep.h>
-#include <libc-pointer-arith.h>
-#include <ldsodefs.h>
-#include "spawn_int.h"
+#include <stddef.h>
+
/* The Linux implementation of posix_spawn{p} uses the clone syscall directly
with CLONE_VM and CLONE_VFORK flags and an allocated stack. The new stack
@@ -52,79 +38,51 @@
code. CLONE_VFORK ensures that the parent does not run until the
child has either exec'ed successfully or exited. */
+/* Due the CLONE_VFORK synchronization (where parent will resume execution
+ only when child ends its own) there is no need of external mechanism to
+ send the error value (such as a pipe for POSIX default implementation). */
+#define _POSIX_SPAWN_SYNC_ARGS \
+ int err
-/* The Unix standard contains a long explanation of the way to signal
- an error after the fork() was successful. Since no new wait status
- was wanted there is no way to signal an error using one of the
- available methods. The committee chose to signal an error by a
- normal program exit with the exit code 127. */
-#define SPAWN_ERROR 127
+#define _POSIX_SPAWN_CHILD_SYNC(__args)
-#ifdef __ia64__
-# define CLONE(__fn, __stackbase, __stacksize, __flags, __args) \
- __clone2 (__fn, __stackbase, __stacksize, __flags, __args, 0, 0, 0)
-#else
-# define CLONE(__fn, __stack, __stacksize, __flags, __args) \
- __clone (__fn, __stack, __flags, __args)
-#endif
+static void __spawni_child_setup_signals (const posix_spawnattr_t
+ *restrict attr);
+#define _POSIX_SPAWN_CHILD_SETUP_SIGNAL(__attr) \
+ __spawni_child_setup_signals (__attr)
-/* Since ia64 wants the stackbase w/clone2, re-use the grows-up macro. */
-#if _STACK_GROWS_UP || defined (__ia64__)
-# define STACK(__stack, __stack_size) (__stack)
-#elif _STACK_GROWS_DOWN
-# define STACK(__stack, __stack_size) (__stack + __stack_size)
-#endif
+/* Initialize the error value just before the execve/execvep call. */
+#define _POSIX_SPAWN_CHILD_SYNC_PRE_EXEC(__args) \
+ __args->err = 0
+/* errno should have an appropriate non-zero value; otherwise,
+ there's a bug in glibc or the kernel. For lack of an error code
+ (EINTERNALBUG) describing that, use ECHILD. Another option would
+ be to set args->err to some negative sentinel and have the parent
+ abort(), but that seems needlessly harsh. */
+#define _POSIX_SPAWN_CHILD_SYNC_END(__ret, __args) \
+ args->err = errno ? : ECHILD
-struct posix_spawn_args
-{
- sigset_t oldmask;
- const char *file;
- int (*exec) (const char *, char *const *, char *const *);
- const posix_spawn_file_actions_t *fa;
- const posix_spawnattr_t *restrict attr;
- char *const *argv;
- ptrdiff_t argc;
- char *const *envp;
- int xflags;
- int err;
-};
-
-/* Older version requires that shell script without shebang definition
- to be called explicitly using /bin/sh (_PATH_BSHELL). */
-static void
-maybe_script_execute (struct posix_spawn_args *args)
-{
- if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
- && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC)
- {
- char *const *argv = args->argv;
- ptrdiff_t argc = args->argc;
-
- /* Construct an argument list for the shell. */
- char *new_argv[argc + 1];
- new_argv[0] = (char *) _PATH_BSHELL;
- new_argv[1] = (char *) args->file;
- if (argc > 1)
- memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
- else
- new_argv[2] = NULL;
+#define _POSIX_SPAWN_PARENT_SYNC_START(__args)
- /* Execute the shell. */
- args->exec (new_argv[0], new_argv, args->envp);
- }
-}
+struct posix_spawn_args;
+static int __spawn_process (pid_t *pid, ptrdiff_t argc,
+ struct posix_spawn_args *args);
+#define _POSIX_SPAWN_PARENT_PROCESS(__pid, __argc, __args) \
+ __spawn_process (__pid, __argc, __args)
+
+#define _POSIX_SPAWN_PARENT_BLOCK_SIGNALS(__args) \
+ __libc_signal_block_all (&__args.oldmask);
+
+#define _POSIX_SPAWN_PARENT_RESTORE_SIGNALS(__args) \
+ __libc_signal_restore_set (&args.oldmask);
+
+#include <sysdeps/posix/spawni.c>
-/* Function used in the clone call to setup the signals mask, posix_spawn
- attributes, and file actions. It run on its own stack (provided by the
- posix_spawn call). */
-static int
-__spawni_child (void *arguments)
-{
- struct posix_spawn_args *args = arguments;
- const posix_spawnattr_t *restrict attr = args->attr;
- const posix_spawn_file_actions_t *file_actions = args->fa;
+static void
+__spawni_child_setup_signals (const posix_spawnattr_t *restrict attr)
+{
/* The child must ensure that no signal handler are enabled because it shared
memory with parent, so the signal disposition must be either SIG_DFL or
SIG_IGN. It does by iterating over all signals and although it could
@@ -160,162 +118,27 @@ __spawni_child (void *arguments)
__libc_sigaction (sig, &sa, 0);
}
+}
-#ifdef _POSIX_PRIORITY_SCHEDULING
- /* Set the scheduling algorithm and parameters. */
- if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
- == POSIX_SPAWN_SETSCHEDPARAM)
- {
- if (__sched_setparam (0, &attr->__sp) == -1)
- goto fail;
- }
- else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0)
- {
- if (__sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)
- goto fail;
- }
-#endif
-
- if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
- && __setsid () < 0)
- goto fail;
-
- /* Set the process group ID. */
- if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
- && __setpgid (0, attr->__pgrp) != 0)
- goto fail;
-
- /* Set the effective user and group IDs. */
- if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0
- && (local_seteuid (__getuid ()) != 0
- || local_setegid (__getgid ()) != 0))
- goto fail;
-
- /* Execute the file actions. */
- if (file_actions != 0)
- {
- int cnt;
- struct rlimit64 fdlimit;
- bool have_fdlimit = false;
-
- for (cnt = 0; cnt < file_actions->__used; ++cnt)
- {
- struct __spawn_action *action = &file_actions->__actions[cnt];
- switch (action->tag)
- {
- case spawn_do_close:
- if (close_not_cancel (action->action.close_action.fd) != 0)
- {
- if (!have_fdlimit)
- {
- __getrlimit64 (RLIMIT_NOFILE, &fdlimit);
- have_fdlimit = true;
- }
-
- /* Signal errors only for file descriptors out of range. */
- if (action->action.close_action.fd < 0
- || action->action.close_action.fd >= fdlimit.rlim_cur)
- goto fail;
- }
- break;
-
- case spawn_do_open:
- {
- /* POSIX states that if fildes was already an open file descriptor,
- it shall be closed before the new file is opened. This avoid
- pontential issues when posix_spawn plus addopen action is called
- with the process already at maximum number of file descriptor
- opened and also for multiple actions on single-open special
- paths (like /dev/watchdog). */
- close_not_cancel (action->action.open_action.fd);
-
- int ret = open_not_cancel (action->action.open_action.path,
- action->action.
- open_action.oflag | O_LARGEFILE,
- action->action.open_action.mode);
-
- if (ret == -1)
- goto fail;
-
- int new_fd = ret;
-
- /* Make sure the desired file descriptor is used. */
- if (ret != action->action.open_action.fd)
- {
- if (__dup2 (new_fd, action->action.open_action.fd)
- != action->action.open_action.fd)
- goto fail;
-
- if (close_not_cancel (new_fd) != 0)
- goto fail;
- }
- }
- break;
-
- case spawn_do_dup2:
- if (__dup2 (action->action.dup2_action.fd,
- action->action.dup2_action.newfd)
- != action->action.dup2_action.newfd)
- goto fail;
- break;
- }
- }
- }
+#ifdef __ia64__
+# define CLONE(__fn, __stackbase, __stacksize, __flags, __args) \
+ __clone2 (__fn, __stackbase, __stacksize, __flags, __args, 0, 0, 0)
+#else
+# define CLONE(__fn, __stack, __stacksize, __flags, __args) \
+ __clone (__fn, __stack, __flags, __args)
+#endif
- /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
- is set, otherwise restore the previous one. */
- __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
- ? &attr->__ss : &args->oldmask, 0);
-
- args->err = 0;
- args->exec (args->file, args->argv, args->envp);
-
- /* This is compatibility function required to enable posix_spawn run
- script without shebang definition for older posix_spawn versions
- (2.15). */
- maybe_script_execute (args);
-
-fail:
- /* errno should have an appropriate non-zero value; otherwise,
- there's a bug in glibc or the kernel. For lack of an error code
- (EINTERNALBUG) describing that, use ECHILD. Another option would
- be to set args->err to some negative sentinel and have the parent
- abort(), but that seems needlessly harsh. */
- args->err = errno ? : ECHILD;
- _exit (SPAWN_ERROR);
-}
+/* Since ia64 wants the stackbase w/clone2, re-use the grows-up macro. */
+#if _STACK_GROWS_UP || defined (__ia64__)
+# define STACK(__stack, __stack_size) (__stack)
+#elif _STACK_GROWS_DOWN
+# define STACK(__stack, __stack_size) (__stack + __stack_size)
+#endif
-/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
- Before running the process perform the actions described in FILE-ACTIONS. */
static int
-__spawnix (pid_t * pid, const char *file,
- const posix_spawn_file_actions_t * file_actions,
- const posix_spawnattr_t * attrp, char *const argv[],
- char *const envp[], int xflags,
- int (*exec) (const char *, char *const *, char *const *))
+__spawn_process (pid_t *pid, ptrdiff_t argc, struct posix_spawn_args *args)
{
- pid_t new_pid;
- struct posix_spawn_args args;
- int ec;
-
- /* To avoid imposing hard limits on posix_spawn{p} the total number of
- arguments is first calculated to allocate a mmap to hold all possible
- values. */
- ptrdiff_t argc = 0;
- /* Linux allows at most max (0x7FFFFFFF, 1/4 stack size) arguments
- to be used in a execve call. We limit to INT_MAX minus one due the
- compatiblity code that may execute a shell script (maybe_script_execute)
- where it will construct another argument list with an additional
- argument. */
- ptrdiff_t limit = INT_MAX - 1;
- while (argv[argc++] != NULL)
- if (argc == limit)
- {
- errno = E2BIG;
- return errno;
- }
-
int prot = (PROT_READ | PROT_WRITE
| ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
@@ -332,25 +155,6 @@ __spawnix (pid_t * pid, const char *file,
if (__glibc_unlikely (stack == MAP_FAILED))
return errno;
- /* Disable asynchronous cancellation. */
- int state;
- __libc_ptf_call (__pthread_setcancelstate,
- (PTHREAD_CANCEL_DISABLE, &state), 0);
-
- /* Child must set args.err to something non-negative - we rely on
- the parent and child sharing VM. */
- args.err = -1;
- args.file = file;
- args.exec = exec;
- args.fa = file_actions;
- args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 };
- args.argv = argv;
- args.argc = argc;
- args.envp = envp;
- args.xflags = xflags;
-
- __libc_signal_block_all (&args.oldmask);
-
/* The clone flags used will create a new child that will run in the same
memory space (CLONE_VM) and the execution of calling thread will be
suspend until the child calls execve or _exit.
@@ -359,39 +163,22 @@ __spawnix (pid_t * pid, const char *file,
need for CLONE_SETTLS. Although parent and child share the same TLS
namespace, there will be no concurrent access for TLS variables (errno
for instance). */
- new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
- CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
+ pid_t new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
+ CLONE_VM | CLONE_VFORK | SIGCHLD, args);
+ int ec = 0;
if (new_pid > 0)
{
- ec = args.err;
+ ec = args->err;
assert (ec >= 0);
if (ec != 0)
- __waitpid (new_pid, NULL, 0);
+ __waitpid (new_pid, NULL, 0);
}
else
ec = -new_pid;
__munmap (stack, stack_size);
- if ((ec == 0) && (pid != NULL))
- *pid = new_pid;
-
- __libc_signal_restore_set (&args.oldmask);
-
- __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
-
+ *pid = new_pid;
return ec;
}
-
-/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
- Before running the process perform the actions described in FILE-ACTIONS. */
-int
-__spawni (pid_t * pid, const char *file,
- const posix_spawn_file_actions_t * acts,
- const posix_spawnattr_t * attrp, char *const argv[],
- char *const envp[], int xflags)
-{
- return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
- xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
-}
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] posix: Refactor posix_spawn
2017-05-15 19:36 ` [PATCH 3/3] posix: Refactor posix_spawn Adhemerval Zanella
@ 2017-06-28 1:07 ` Rasmus Villemoes
2017-06-28 12:51 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Rasmus Villemoes @ 2017-06-28 1:07 UTC (permalink / raw)
To: libc-alpha
On Mon, May 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> This patch refactor both POSIX and Linux internal posix_spawn to use
> a common implementation. The POSIX one is parametrized to use define
> hooks and Linux reimplements it on its file. No functional change is
> expected.
>
Hm, I do think this makes the code quite a bit harder to understand,
since one has to skip back and forth between the #includer and the file
with the bulk of the implementation to see what actually happens.
I didn't read it all carefully, but there were at least a number of places
where your macro parameters have a double leading underscore, but then
you omit those in the macro body. I don't suppose that's intentional?
>
> +/* errno should have an appropriate non-zero value; otherwise,
> + there's a bug in glibc or the kernel. For lack of an error code
> + (EINTERNALBUG) describing that, use ECHILD. Another option would
> + be to set args->err to some negative sentinel and have the parent
> + abort(), but that seems needlessly harsh. */
> +#define _POSIX_SPAWN_CHILD_SYNC_END(__ret, __args) \
> + args->err = errno ? : ECHILD
here
> +struct posix_spawn_args;
> +static int __spawn_process (pid_t *pid, ptrdiff_t argc,
> + struct posix_spawn_args *args);
> +#define _POSIX_SPAWN_PARENT_PROCESS(__pid, __argc, __args) \
> + __spawn_process (__pid, __argc, __args)
> +
> +#define _POSIX_SPAWN_PARENT_BLOCK_SIGNALS(__args) \
> + __libc_signal_block_all (&__args.oldmask);
> +
> +#define _POSIX_SPAWN_PARENT_RESTORE_SIGNALS(__args) \
> + __libc_signal_restore_set (&args.oldmask);
here
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport.
2017-05-15 19:36 [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 3/3] posix: Refactor posix_spawn Adhemerval Zanella
@ 2017-06-28 12:47 ` Adhemerval Zanella
2 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-06-28 12:47 UTC (permalink / raw)
To: libc-alpha
Any objections about this patch? Otherwise I will push it in following hours.
On 15/05/2017 16:36, Adhemerval Zanella wrote:
> Checked on x86_64-linux-gnu.
>
> * posix/tst-spawn2.c (do_test): Use libsupport.
> * posix/tst-spawn3.c (do_test): Likewise.
> ---
> ChangeLog | 5 ++++
> posix/tst-spawn2.c | 26 +++++++++-------
> posix/tst-spawn3.c | 88 ++++++++++++++++--------------------------------------
> 3 files changed, 46 insertions(+), 73 deletions(-)
>
> diff --git a/posix/tst-spawn2.c b/posix/tst-spawn2.c
> index 73a37b6..3a2e041 100644
> --- a/posix/tst-spawn2.c
> +++ b/posix/tst-spawn2.c
> @@ -23,11 +23,12 @@
> #include <string.h>
> #include <unistd.h>
> #include <sys/wait.h>
> -
> #include <stdio.h>
>
> +#include <support/check.h>
> +
> int
> -posix_spawn_test (void)
> +do_test (void)
> {
> /* Check if posix_spawn correctly returns an error and an invalid pid
> by trying to spawn an invalid binary. */
> @@ -38,35 +39,40 @@ posix_spawn_test (void)
>
> int ret = posix_spawn (&pid, program, 0, 0, args, environ);
> if (ret != ENOENT)
> - error (EXIT_FAILURE, errno, "posix_spawn");
> + {
> + errno = ret;
> + FAIL_EXIT1 ("posix_spawn: %m");
> + }
>
> /* POSIX states the value returned on pid variable in case of an error
> is not specified. GLIBC will update the value iff the child
> execution is successful. */
> if (pid != -1)
> - error (EXIT_FAILURE, errno, "posix_spawn returned pid != -1");
> + FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
>
> /* Check if no child is actually created. */
> ret = waitpid (-1, NULL, 0);
> if (ret != -1 || errno != ECHILD)
> - error (EXIT_FAILURE, errno, "waitpid");
> + FAIL_EXIT1 ("waitpid: %m)");
>
> /* Same as before, but with posix_spawnp. */
> char *args2[] = { (char*) program, 0 };
>
> ret = posix_spawnp (&pid, args2[0], 0, 0, args2, environ);
> if (ret != ENOENT)
> - error (EXIT_FAILURE, errno, "posix_spawnp");
> + {
> + errno = ret;
> + FAIL_EXIT1 ("posix_spawnp: %m");
> + }
>
> if (pid != -1)
> - error (EXIT_FAILURE, errno, "posix_spawnp returned pid != -1");
> + FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
>
> ret = waitpid (-1, NULL, 0);
> if (ret != -1 || errno != ECHILD)
> - error (EXIT_FAILURE, errno, "waitpid");
> + FAIL_EXIT1 ("waitpid: %m)");
>
> return 0;
> }
>
> -#define TEST_FUNCTION posix_spawn_test ()
> -#include "../test-skeleton.c"
> +#include <support/test-driver.c>
> diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c
> index 8577b03..28a4872 100644
> --- a/posix/tst-spawn3.c
> +++ b/posix/tst-spawn3.c
> @@ -25,10 +25,11 @@
> #include <unistd.h>
> #include <sys/wait.h>
> #include <sys/resource.h>
> +#include <fcntl.h>
> +#include <paths.h>
>
> -static int do_test (void);
> -#define TEST_FUNCTION do_test ()
> -#include <test-skeleton.c>
> +#include <support/check.h>
> +#include <support/temp_file.h>
>
> static int
> do_test (void)
> @@ -47,25 +48,20 @@ do_test (void)
>
> struct rlimit rl;
> int max_fd = 24;
> + int ret;
>
> /* Set maximum number of file descriptor to a low value to avoid open
> too many files in environments where RLIMIT_NOFILE is large and to
> limit the array size to track the opened file descriptors. */
>
> if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
> - {
> - printf ("error: getrlimit RLIMIT_NOFILE failed");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
>
> max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd);
> rl.rlim_cur = max_fd;
>
> if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
> - {
> - printf ("error: setrlimit RLIMIT_NOFILE to %u failed", max_fd);
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
>
> /* Exhauste the file descriptor limit with temporary files. */
> int files[max_fd];
> @@ -76,11 +72,7 @@ do_test (void)
> if (fd == -1)
> {
> if (errno != EMFILE)
> - {
> - printf ("error: create_temp_file returned -1 with "
> - "errno != EMFILE\n");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("create_temp_file: %m");
> break;
> }
> files[nfiles++] = fd;
> @@ -88,25 +80,16 @@ do_test (void)
>
> posix_spawn_file_actions_t a;
> if (posix_spawn_file_actions_init (&a) != 0)
> - {
> - puts ("error: spawn_file_actions_init failed");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("posix_spawn_file_actions_init");
>
> /* Executes a /bin/sh echo $$ 2>&1 > /tmp/tst-spawn3.pid . */
> const char pidfile[] = "/tmp/tst-spawn3.pid";
> if (posix_spawn_file_actions_addopen (&a, STDOUT_FILENO, pidfile, O_WRONLY |
> O_CREAT | O_TRUNC, 0644) != 0)
> - {
> - puts ("error: spawn_file_actions_addopen failed");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("posix_spawn_file_actions_addopen");
>
> if (posix_spawn_file_actions_adddup2 (&a, STDOUT_FILENO, STDERR_FILENO) != 0)
> - {
> - puts ("error: spawn_file_actions_addclose");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("posix_spawn_file_actions_adddup2");
>
> /* Since execve (called by posix_spawn) might require to open files to
> actually execute the shell script, setup to close the temporary file
> @@ -114,54 +97,40 @@ do_test (void)
> for (int i=0; i<nfiles; i++)
> {
> if (posix_spawn_file_actions_addclose (&a, files[i]))
> - {
> - printf ("error: posix_spawn_file_actions_addclose failed");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("posix_spawn_file_actions_addclose");
> }
>
> char *spawn_argv[] = { (char *) _PATH_BSHELL, (char *) "-c",
> (char *) "echo $$", NULL };
> pid_t pid;
> - if (posix_spawn (&pid, _PATH_BSHELL, &a, NULL, spawn_argv, NULL) != 0)
> + if ((ret = posix_spawn (&pid, _PATH_BSHELL, &a, NULL, spawn_argv, NULL))
> + != 0)
> {
> - puts ("error: posix_spawn failed");
> - exit (EXIT_FAILURE);
> + errno = ret;
> + FAIL_EXIT1 ("posix_spawn: %m");
> }
>
> int status;
> int err = waitpid (pid, &status, 0);
> if (err != pid)
> - {
> - puts ("error: waitpid failed");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("waitpid: %m");
>
> /* Close the temporary files descriptor so it can check posix_spawn
> output. */
> for (int i=0; i<nfiles; i++)
> {
> if (close (files[i]))
> - {
> - printf ("error: close failed\n");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("close: %m");
> }
>
> int pidfd = open (pidfile, O_RDONLY);
> if (pidfd == -1)
> - {
> - printf ("error: open pidfile failed\n");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("open: %m");
>
> char buf[64];
> ssize_t n;
> if ((n = read (pidfd, buf, sizeof (buf))) < 0)
> - {
> - printf ("error: read pidfile failed\n");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("read: %m");
>
> unlink (pidfile);
>
> @@ -169,21 +138,14 @@ do_test (void)
> char *endp;
> long int rpid = strtol (buf, &endp, 10);
> if (*endp != '\n')
> - {
> - printf ("error: didn't parse whole line: \"%s\"\n", buf);
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("*endp != \'n\'");
> if (endp == buf)
> - {
> - puts ("error: read empty line");
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("read empty line");
>
> if (rpid != pid)
> - {
> - printf ("error: found \"%s\", expected PID %ld\n", buf, (long int) pid);
> - exit (EXIT_FAILURE);
> - }
> + FAIL_EXIT1 ("found \"%s\", expected pid %ld\n", buf, (long int) pid);
>
> return 0;
> }
> +
> +#include <support/test-driver.c>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] posix: Refactor posix_spawn
2017-06-28 1:07 ` Rasmus Villemoes
@ 2017-06-28 12:51 ` Adhemerval Zanella
0 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-06-28 12:51 UTC (permalink / raw)
To: libc-alpha
On 27/06/2017 22:07, Rasmus Villemoes wrote:
> On Mon, May 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> This patch refactor both POSIX and Linux internal posix_spawn to use
>> a common implementation. The POSIX one is parametrized to use define
>> hooks and Linux reimplements it on its file. No functional change is
>> expected.
>>
>
> Hm, I do think this makes the code quite a bit harder to understand,
> since one has to skip back and forth between the #includer and the file
> with the bulk of the implementation to see what actually happens.
>
> I didn't read it all carefully, but there were at least a number of places
> where your macro parameters have a double leading underscore, but then
> you omit those in the macro body. I don't suppose that's intentional?
I was in fact unsure about this change and thinking twice now I am not
sure if it indeed adds more readability. My idea was to try share as
much code is possible with the default posix_spawn implementation,
but it ended with a somewhat convoluted code with a lot of #includes.
I am tending to just drop this specific change and just push the
posix implementation (patch 2/3).
>
>>
>> +/* errno should have an appropriate non-zero value; otherwise,
>> + there's a bug in glibc or the kernel. For lack of an error code
>> + (EINTERNALBUG) describing that, use ECHILD. Another option would
>> + be to set args->err to some negative sentinel and have the parent
>> + abort(), but that seems needlessly harsh. */
>> +#define _POSIX_SPAWN_CHILD_SYNC_END(__ret, __args) \
>> + args->err = errno ? : ECHILD
>
> here
>
>> +struct posix_spawn_args;
>> +static int __spawn_process (pid_t *pid, ptrdiff_t argc,
>> + struct posix_spawn_args *args);
>> +#define _POSIX_SPAWN_PARENT_PROCESS(__pid, __argc, __args) \
>> + __spawn_process (__pid, __argc, __args)
>> +
>> +#define _POSIX_SPAWN_PARENT_BLOCK_SIGNALS(__args) \
>> + __libc_signal_block_all (&__args.oldmask);
>> +
>> +#define _POSIX_SPAWN_PARENT_RESTORE_SIGNALS(__args) \
>> + __libc_signal_restore_set (&args.oldmask);
>
> here
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
@ 2017-06-28 20:27 ` Adhemerval Zanella
2017-06-29 14:22 ` Andreas Schwab
1 sibling, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-06-28 20:27 UTC (permalink / raw)
To: libc-alpha
Since this is not used in any port currently (since hurd uses its own
implementation which I think suffers from the same issue of the current
posix one) and this patch just align the posix one to Linux recent
fixes I will commit this shortly if no one objects.
On 15/05/2017 16:36, Adhemerval Zanella wrote:
> This patch improves the default posix implementation of posix_spawn{p}
> and align with Linux one. The main idea is to try shared common
> implementation bits with Linux and avoid code duplication, fix some
> issues already fixed in Linux code, and deprecated vfork internal
> usage (source of various bug reports). In a short:
>
> - It moves POSIX_SPAWN_USEVFORK usage and sets it a no-op. Since
> the process that actually spawn the new process do not share
> memory with parent (with vfork), it fixes BZ#14750 for this
> implementation.
>
> - It uses a pipe to correctly obtain the return upon failure
> of execution (BZ#18433).
>
> - It correctly enable/disable asynchronous cancellation (checked
> on ptl/tst-exec5.c).
>
> - It correctly disable/enable signal handling.
>
> Using this version instead of Linux shows only one regression,
> posix/tst-spawn3, because of pipe2 usage which increase total
> number of file descriptor.
>
> * sysdeps/posix/spawni.c (__spawni_child): New function.
> (__spawni): Rename to __spawnix.
> ---
> ChangeLog | 3 +
> sysdeps/posix/spawni.c | 362 ++++++++++++++++++++++++-------------------------
> 2 files changed, 182 insertions(+), 183 deletions(-)
>
> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
> index 9cad25c..e096a42 100644
> --- a/sysdeps/posix/spawni.c
> +++ b/sysdeps/posix/spawni.c
> @@ -16,20 +16,23 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> -#include <errno.h>
> +#include <spawn.h>
> +#include <assert.h>
> #include <fcntl.h>
> #include <paths.h>
> -#include <spawn.h>
> -#include <stdbool.h>
> -#include <stdlib.h>
> #include <string.h>
> -#include <unistd.h>
> -#include <signal.h>
> #include <sys/resource.h>
> -#include "spawn_int.h"
> +#include <sys/wait.h>
> +#include <sys/param.h>
> +#include <sys/mman.h>
> #include <not-cancel.h>
> #include <local-setxid.h>
> #include <shlib-compat.h>
> +#include <nptl/pthreadP.h>
> +#include <dl-sysdep.h>
> +#include <libc-pointer-arith.h>
> +#include <ldsodefs.h>
> +#include "spawn_int.h"
>
>
> /* The Unix standard contains a long explanation of the way to signal
> @@ -39,94 +42,59 @@
> normal program exit with the exit code 127. */
> #define SPAWN_ERROR 127
>
> -
> -/* The file is accessible but it is not an executable file. Invoke
> - the shell to interpret it as a script. */
> -static void
> -internal_function
> -script_execute (const char *file, char *const argv[], char *const envp[])
> +struct posix_spawn_args
> {
> - /* Count the arguments. */
> - int argc = 0;
> - while (argv[argc++])
> - ;
> -
> - /* Construct an argument list for the shell. */
> - {
> - char *new_argv[argc + 1];
> - new_argv[0] = (char *) _PATH_BSHELL;
> - new_argv[1] = (char *) file;
> - while (argc > 1)
> - {
> - new_argv[argc] = argv[argc - 1];
> - --argc;
> - }
> -
> - /* Execute the shell. */
> - __execve (new_argv[0], new_argv, envp);
> - }
> -}
> -
> -static inline void
> -maybe_script_execute (const char *file, char *const argv[], char *const envp[],
> - int xflags)
> + sigset_t oldmask;
> + const char *file;
> + int (*exec) (const char *, char *const *, char *const *);
> + const posix_spawn_file_actions_t *fa;
> + const posix_spawnattr_t *restrict attr;
> + char *const *argv;
> + ptrdiff_t argc;
> + char *const *envp;
> + int xflags;
> + int pipe[2];
> +};
> +
> +/* Older version requires that shell script without shebang definition
> + to be called explicitly using /bin/sh (_PATH_BSHELL). */
> +static void
> +maybe_script_execute (struct posix_spawn_args *args)
> {
> if (SHLIB_COMPAT (libc, GLIBC_2_2, GLIBC_2_15)
> - && (xflags & SPAWN_XFLAGS_TRY_SHELL)
> - && errno == ENOEXEC)
> - script_execute (file, argv, envp);
> -}
> -
> -/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
> - Before running the process perform the actions described in FILE-ACTIONS. */
> -int
> -__spawni (pid_t *pid, const char *file,
> - const posix_spawn_file_actions_t *file_actions,
> - const posix_spawnattr_t *attrp, char *const argv[],
> - char *const envp[], int xflags)
> -{
> - pid_t new_pid;
> - char *path, *p, *name;
> - size_t len;
> - size_t pathlen;
> -
> - /* Do this once. */
> - short int flags = attrp == NULL ? 0 : attrp->__flags;
> -
> - /* Generate the new process. */
> - if ((flags & POSIX_SPAWN_USEVFORK) != 0
> - /* If no major work is done, allow using vfork. Note that we
> - might perform the path searching. But this would be done by
> - a call to execvp(), too, and such a call must be OK according
> - to POSIX. */
> - || ((flags & (POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF
> - | POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER
> - | POSIX_SPAWN_SETPGROUP | POSIX_SPAWN_RESETIDS
> - | POSIX_SPAWN_SETSID)) == 0
> - && file_actions == NULL))
> - new_pid = __vfork ();
> - else
> - new_pid = __fork ();
> -
> - if (new_pid != 0)
> + && (args->xflags & SPAWN_XFLAGS_TRY_SHELL) && errno == ENOEXEC)
> {
> - if (new_pid < 0)
> - return errno;
> -
> - /* The call was successful. Store the PID if necessary. */
> - if (pid != NULL)
> - *pid = new_pid;
> + char *const *argv = args->argv;
> + ptrdiff_t argc = args->argc;
> +
> + /* Construct an argument list for the shell. */
> + char *new_argv[argc + 1];
> + new_argv[0] = (char *) _PATH_BSHELL;
> + new_argv[1] = (char *) args->file;
> + if (argc > 1)
> + memcpy (new_argv + 2, argv + 1, argc * sizeof(char *));
> + else
> + new_argv[2] = NULL;
>
> - return 0;
> + /* Execute the shell. */
> + args->exec (new_argv[0], new_argv, args->envp);
> }
> +}
> +
> +/* Function used in the clone call to setup the signals mask, posix_spawn
> + attributes, and file actions. */
> +static int
> +__spawni_child (void *arguments)
> +{
> + struct posix_spawn_args *args = arguments;
> + const posix_spawnattr_t *restrict attr = args->attr;
> + const posix_spawn_file_actions_t *file_actions = args->fa;
> + int ret;
>
> - /* Set signal mask. */
> - if ((flags & POSIX_SPAWN_SETSIGMASK) != 0
> - && __sigprocmask (SIG_SETMASK, &attrp->__ss, NULL) != 0)
> - _exit (SPAWN_ERROR);
> + __close (args->pipe[0]);
>
> /* Set signal default action. */
> - if ((flags & POSIX_SPAWN_SETSIGDEF) != 0)
> + if ((attr->__flags & POSIX_SPAWN_SETSIGDEF) != 0)
> {
> /* We have to iterate over all signals. This could possibly be
> done better but it requires system specific solutions since
> @@ -139,41 +107,41 @@ __spawni (pid_t *pid, const char *file,
> sa.sa_handler = SIG_DFL;
>
> for (sig = 1; sig <= _NSIG; ++sig)
> - if (__sigismember (&attrp->__sd, sig) != 0
> + if (__sigismember (&attr->__sd, sig) != 0
> && __sigaction (sig, &sa, NULL) != 0)
> - _exit (SPAWN_ERROR);
> -
> + goto fail;
> }
>
> #ifdef _POSIX_PRIORITY_SCHEDULING
> /* Set the scheduling algorithm and parameters. */
> - if ((flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
> + if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
> == POSIX_SPAWN_SETSCHEDPARAM)
> {
> - if (__sched_setparam (0, &attrp->__sp) == -1)
> - _exit (SPAWN_ERROR);
> + if ((ret = __sched_setparam (0, &attr->__sp)) == -1)
> + goto fail;
> }
> - else if ((flags & POSIX_SPAWN_SETSCHEDULER) != 0)
> + else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0)
> {
> - if (__sched_setscheduler (0, attrp->__policy, &attrp->__sp) == -1)
> - _exit (SPAWN_ERROR);
> + if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1))
> + goto fail;
> }
> #endif
>
> - if ((flags & POSIX_SPAWN_SETSID) != 0
> - && __setsid () < 0)
> - _exit (SPAWN_ERROR);
> + /* Set the process session ID. */
> + if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
> + && (ret = __setsid ()) < 0)
> + goto fail;
>
> /* Set the process group ID. */
> - if ((flags & POSIX_SPAWN_SETPGROUP) != 0
> - && __setpgid (0, attrp->__pgrp) != 0)
> - _exit (SPAWN_ERROR);
> + if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
> + && (ret =__setpgid (0, attr->__pgrp)) != 0)
> + goto fail;
>
> /* Set the effective user and group IDs. */
> - if ((flags & POSIX_SPAWN_RESETIDS) != 0
> - && (local_seteuid (__getuid ()) != 0
> - || local_setegid (__getgid ()) != 0))
> - _exit (SPAWN_ERROR);
> + if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0
> + && ((ret = local_seteuid (__getuid ())) != 0
> + || (ret = local_setegid (__getgid ())) != 0))
> + goto fail;
>
> /* Execute the file actions. */
> if (file_actions != NULL)
> @@ -191,7 +159,7 @@ __spawni (pid_t *pid, const char *file,
> case spawn_do_close:
> if (close_not_cancel (action->action.close_action.fd) != 0)
> {
> - if (! have_fdlimit)
> + if (have_fdlimit == 0)
> {
> __getrlimit64 (RLIMIT_NOFILE, &fdlimit);
> have_fdlimit = true;
> @@ -200,120 +168,148 @@ __spawni (pid_t *pid, const char *file,
> /* Only signal errors for file descriptors out of range. */
> if (action->action.close_action.fd < 0
> || action->action.close_action.fd >= fdlimit.rlim_cur)
> - /* Signal the error. */
> - _exit (SPAWN_ERROR);
> + {
> + ret = -1;
> + goto fail;
> + }
> }
> break;
>
> case spawn_do_open:
> {
> + /* POSIX states that if fildes was already an open file descriptor,
> + it shall be closed before the new file is opened. This avoid
> + pontential issues when posix_spawn plus addopen action is called
> + with the process already at maximum number of file descriptor
> + opened and also for multiple actions on single-open special
> + paths (like /dev/watchdog). */
> + close_not_cancel (action->action.open_action.fd);
> +
> int new_fd = open_not_cancel (action->action.open_action.path,
> action->action.open_action.oflag
> | O_LARGEFILE,
> action->action.open_action.mode);
>
> - if (new_fd == -1)
> - /* The `open' call failed. */
> - _exit (SPAWN_ERROR);
> + if ((ret = new_fd) == -1)
> + goto fail;
>
> /* Make sure the desired file descriptor is used. */
> if (new_fd != action->action.open_action.fd)
> {
> - if (__dup2 (new_fd, action->action.open_action.fd)
> + if ((ret = __dup2 (new_fd, action->action.open_action.fd))
> != action->action.open_action.fd)
> - /* The `dup2' call failed. */
> - _exit (SPAWN_ERROR);
> + goto fail;
>
> - if (close_not_cancel (new_fd) != 0)
> - /* The `close' call failed. */
> - _exit (SPAWN_ERROR);
> + if ((ret = close_not_cancel (new_fd) != 0))
> + goto fail;
> }
> }
> break;
>
> case spawn_do_dup2:
> - if (__dup2 (action->action.dup2_action.fd,
> - action->action.dup2_action.newfd)
> + if ((ret = __dup2 (action->action.dup2_action.fd,
> + action->action.dup2_action.newfd))
> != action->action.dup2_action.newfd)
> - /* The `dup2' call failed. */
> - _exit (SPAWN_ERROR);
> + goto fail;
> break;
> }
> }
> }
>
> - if ((xflags & SPAWN_XFLAGS_USE_PATH) == 0 || strchr (file, '/') != NULL)
> - {
> - /* The FILE parameter is actually a path. */
> - __execve (file, argv, envp);
> + /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
> + is set, otherwise restore the previous one. */
> + __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
> + ? &attr->__ss : &args->oldmask, 0);
>
> - maybe_script_execute (file, argv, envp, xflags);
> + args->exec (args->file, args->argv, args->envp);
>
> - /* Oh, oh. `execve' returns. This is bad. */
> - _exit (SPAWN_ERROR);
> - }
> + /* This is compatibility function required to enable posix_spawn run
> + script without shebang definition for older posix_spawn versions
> + (2.15). */
> + maybe_script_execute (args);
>
> - /* We have to search for FILE on the path. */
> - path = getenv ("PATH");
> - if (path == NULL)
> - {
> - /* There is no `PATH' in the environment.
> - The default search path is the current directory
> - followed by the path `confstr' returns for `_CS_PATH'. */
> - len = confstr (_CS_PATH, (char *) NULL, 0);
> - path = (char *) __alloca (1 + len);
> - path[0] = ':';
> - (void) confstr (_CS_PATH, path + 1, len);
> - }
> + ret = -errno;
>
> - len = strlen (file) + 1;
> - pathlen = strlen (path);
> - name = __alloca (pathlen + len + 1);
> - /* Copy the file name at the top. */
> - name = (char *) memcpy (name + pathlen + 1, file, len);
> - /* And add the slash. */
> - *--name = '/';
> +fail:
> + /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> + ret = -ret;
> + if (ret)
> + while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0);
>
> - p = path;
> - do
> - {
> - char *startp;
> + _exit (SPAWN_ERROR);
> +}
>
> - path = p;
> - p = __strchrnul (path, ':');
> +/* Spawn a new process executing PATH with the attributes describes in *ATTRP.
> + Before running the process perform the actions described in FILE-ACTIONS. */
> +int
> +__spawnix (pid_t *pid, const char *file,
> + const posix_spawn_file_actions_t *file_actions,
> + const posix_spawnattr_t *attrp, char *const argv[],
> + char *const envp[], int xflags,
> + int (*exec) (const char *, char *const *, char *const *))
> +{
> + struct posix_spawn_args args;
> + int ec;
>
> - if (p == path)
> - /* Two adjacent colons, or a colon at the beginning or the end
> - of `PATH' means to search the current directory. */
> - startp = name + 1;
> - else
> - startp = (char *) memcpy (name - (p - path), path, p - path);
> + if (__pipe2 (args.pipe, O_CLOEXEC))
> + return errno;
>
> - /* Try to execute this name. If it works, execv will not return. */
> - __execve (startp, argv, envp);
> + /* Disable asynchronous cancellation. */
> + int state;
> + __libc_ptf_call (__pthread_setcancelstate,
> + (PTHREAD_CANCEL_DISABLE, &state), 0);
>
> - maybe_script_execute (startp, argv, envp, xflags);
> + ptrdiff_t argc = 0;
> + ptrdiff_t limit = INT_MAX - 1;
> + while (argv[argc++] != NULL)
> + if (argc == limit)
> + {
> + errno = E2BIG;
> + return errno;
> + }
>
> - switch (errno)
> - {
> - case EACCES:
> - case ENOENT:
> - case ESTALE:
> - case ENOTDIR:
> - /* Those errors indicate the file is missing or not executable
> - by us, in which case we want to just try the next path
> - directory. */
> - break;
> -
> - default:
> - /* Some other error means we found an executable file, but
> - something went wrong executing it; return the error to our
> - caller. */
> - _exit (SPAWN_ERROR);
> - }
> + args.file = file;
> + args.exec = exec;
> + args.fa = file_actions;
> + args.attr = attrp ? attrp : &(const posix_spawnattr_t) { 0 };
> + args.argv = argv;
> + args.argc = argc;
> + args.envp = envp;
> + args.xflags = xflags;
> +
> + /* Generate the new process. */
> + pid_t new_pid = __fork ();
> +
> + if (new_pid == 0)
> + __spawni_child (&args);
> + else if (new_pid > 0)
> + {
> + __close (args.pipe[1]);
> +
> + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
> + ec = 0;
> + else
> + __waitpid (new_pid, &(int) { 0 }, 0);
> }
> - while (*p++ != '\0');
> + else
> + ec = -new_pid;
>
> - /* Return with an error. */
> - _exit (SPAWN_ERROR);
> + __close (args.pipe[0]);
> +
> + if ((ec == 0) && (pid != NULL))
> + *pid = new_pid;
> +
> + __libc_ptf_call (__pthread_setcancelstate, (state, NULL), 0);
> +
> + return ec;
> +}
> +
> +int
> +__spawni (pid_t * pid, const char *file,
> + const posix_spawn_file_actions_t * acts,
> + const posix_spawnattr_t * attrp, char *const argv[],
> + char *const envp[], int xflags)
> +{
> + return __spawnix (pid, file, acts, attrp, argv, envp, xflags,
> + xflags & SPAWN_XFLAGS_USE_PATH ? __execvpe : __execve);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
2017-06-28 20:27 ` Adhemerval Zanella
@ 2017-06-29 14:22 ` Andreas Schwab
2017-06-29 15:01 ` Adhemerval Zanella
1 sibling, 1 reply; 15+ messages in thread
From: Andreas Schwab @ 2017-06-29 14:22 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Mai 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> + if ((ret = close_not_cancel (new_fd) != 0))
This assigns the wrong value to ret.
> +fail:
> + /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> + ret = -ret;
posix_spwan is supposed to return errno, but none of the arms going here
set ret appropriately.
> + /* Generate the new process. */
> + pid_t new_pid = __fork ();
> +
> + if (new_pid == 0)
> + __spawni_child (&args);
> + else if (new_pid > 0)
> + {
> + __close (args.pipe[1]);
> +
> + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
> + ec = 0;
> + else
> + __waitpid (new_pid, &(int) { 0 }, 0);
> }
> - while (*p++ != '\0');
> + else
> + ec = -new_pid;
new_pid isn't an errno either.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-06-29 14:22 ` Andreas Schwab
@ 2017-06-29 15:01 ` Adhemerval Zanella
2017-06-29 15:20 ` Andreas Schwab
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2017-06-29 15:01 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 29/06/2017 11:22, Andreas Schwab wrote:
> On Mai 15 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> + if ((ret = close_not_cancel (new_fd) != 0))
>
> This assigns the wrong value to ret.
Ugh, this should not be here...
>
>> +fail:
>> + /* Since sizeof errno < PIPE_BUF, the write is atomic. */
>> + ret = -ret;
>
> posix_spwan is supposed to return errno, but none of the arms going here
> set ret appropriately.
Fixed below.
>
>> + /* Generate the new process. */
>> + pid_t new_pid = __fork ();
>> +
>> + if (new_pid == 0)
>> + __spawni_child (&args);
>> + else if (new_pid > 0)
>> + {
>> + __close (args.pipe[1]);
>> +
>> + if (__read (args.pipe[0], &ec, sizeof ec) != sizeof ec)
>> + ec = 0;
>> + else
>> + __waitpid (new_pid, &(int) { 0 }, 0);
>> }
>> - while (*p++ != '\0');
>> + else
>> + ec = -new_pid;
>
> new_pid isn't an errno either.
Since posix_spawn is not support to set errno in case of failure we will
need to save/restore for fork call. And this should be fixed on Linux
implementation as well since clone will also clobber errno in case of
an error (I will send a fix).
Ideally I think the exported clone should not set errno and just return
errno as a negative number.
>
> Andreas.
>
I intended to push this change:
diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
index 1979830..93767a2 100644
--- a/sysdeps/posix/spawni.c
+++ b/sysdeps/posix/spawni.c
@@ -117,30 +117,30 @@ __spawni_child (void *arguments)
if ((attr->__flags & (POSIX_SPAWN_SETSCHEDPARAM | POSIX_SPAWN_SETSCHEDULER))
== POSIX_SPAWN_SETSCHEDPARAM)
{
- if ((ret = __sched_setparam (0, &attr->__sp)) == -1)
+ if (__sched_setparam (0, &attr->__sp) == -1)
goto fail;
}
else if ((attr->__flags & POSIX_SPAWN_SETSCHEDULER) != 0)
{
- if ((ret = __sched_setscheduler (0, attr->__policy, &attr->__sp) == -1))
+ if (__sched_setscheduler (0, attr->__policy, &attr->__sp) == -1)
goto fail;
}
#endif
/* Set the process session ID. */
if ((attr->__flags & POSIX_SPAWN_SETSID) != 0
- && (ret = __setsid ()) < 0)
+ && __setsid () < 0)
goto fail;
/* Set the process group ID. */
if ((attr->__flags & POSIX_SPAWN_SETPGROUP) != 0
- && (ret =__setpgid (0, attr->__pgrp)) != 0)
+ && __setpgid (0, attr->__pgrp) != 0)
goto fail;
/* Set the effective user and group IDs. */
if ((attr->__flags & POSIX_SPAWN_RESETIDS) != 0
- && ((ret = local_seteuid (__getuid ())) != 0
- || (ret = local_setegid (__getgid ())) != 0))
+ && (local_seteuid (__getuid ()) != 0
+ || local_setegid (__getgid ())) != 0)
goto fail;
/* Execute the file actions. */
@@ -168,10 +168,7 @@ __spawni_child (void *arguments)
/* Only signal errors for file descriptors out of range. */
if (action->action.close_action.fd < 0
|| action->action.close_action.fd >= fdlimit.rlim_cur)
- {
- ret = -1;
- goto fail;
- }
+ goto fail;
}
break;
@@ -190,25 +187,25 @@ __spawni_child (void *arguments)
| O_LARGEFILE,
action->action.open_action.mode);
- if ((ret = new_fd) == -1)
+ if (new_fd == -1)
goto fail;
/* Make sure the desired file descriptor is used. */
if (new_fd != action->action.open_action.fd)
{
- if ((ret = __dup2 (new_fd, action->action.open_action.fd))
+ if (__dup2 (new_fd, action->action.open_action.fd)
!= action->action.open_action.fd)
goto fail;
- if ((ret = close_not_cancel (new_fd) != 0))
+ if (close_not_cancel (new_fd) != 0)
goto fail;
}
}
break;
case spawn_do_dup2:
- if ((ret = __dup2 (action->action.dup2_action.fd,
- action->action.dup2_action.newfd))
+ if (__dup2 (action->action.dup2_action.fd,
+ action->action.dup2_action.newfd)
!= action->action.dup2_action.newfd)
goto fail;
break;
@@ -228,12 +225,15 @@ __spawni_child (void *arguments)
(2.15). */
maybe_script_execute (args);
- ret = -errno;
-
fail:
- /* Since sizeof errno < PIPE_BUF, the write is atomic. */
- ret = -ret;
+ /* errno should have an appropriate non-zero value; otherwise,
+ there's a bug in glibc or the kernel. For lack of an error code
+ (EINTERNALBUG) describing that, use ECHILD. Another option would
+ be to set args->err to some negative sentinel and have the parent
+ abort(), but that seems needlessly harsh. */
+ ret = errno ? : ECHILD;
if (ret)
+ /* Since sizeof errno < PIPE_BUF, the write is atomic. */
while (write_not_cancel (args->pipe[1], &ret, sizeof (ret)) < 0);
_exit (SPAWN_ERROR);
@@ -278,6 +278,7 @@ __spawnix (pid_t *pid, const char *file,
args.xflags = xflags;
/* Generate the new process. */
+ int old_errno = errno;
pid_t new_pid = __fork ();
if (new_pid == 0)
@@ -292,7 +293,8 @@ __spawnix (pid_t *pid, const char *file,
__waitpid (new_pid, &(int) { 0 }, 0);
}
else
- ec = -new_pid;
+ ec = errno;
+ errno = old_errno;
__close (args.pipe[0]);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-06-29 15:01 ` Adhemerval Zanella
@ 2017-06-29 15:20 ` Andreas Schwab
2017-06-29 17:39 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Schwab @ 2017-06-29 15:20 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> Since posix_spawn is not support to set errno in case of failure
Is it?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-06-29 15:20 ` Andreas Schwab
@ 2017-06-29 17:39 ` Adhemerval Zanella
2017-07-03 7:08 ` Andreas Schwab
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2017-06-29 17:39 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 29/06/2017 12:20, Andreas Schwab wrote:
> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> Since posix_spawn is not support to set errno in case of failure
>
> Is it?
My understanding of posix_spawn return value [1] states that the error number
should be returned as the function return value, not on errno.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-06-29 17:39 ` Adhemerval Zanella
@ 2017-07-03 7:08 ` Andreas Schwab
2017-07-03 12:10 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Schwab @ 2017-07-03 7:08 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> On 29/06/2017 12:20, Andreas Schwab wrote:
>> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>
>>> Since posix_spawn is not support to set errno in case of failure
>>
>> Is it?
>
> My understanding of posix_spawn return value [1] states that the error number
> should be returned as the function return value, not on errno.
But that doesn't forbid spurious writes to errno, does it?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-07-03 7:08 ` Andreas Schwab
@ 2017-07-03 12:10 ` Adhemerval Zanella
2017-07-03 13:02 ` Andreas Schwab
0 siblings, 1 reply; 15+ messages in thread
From: Adhemerval Zanella @ 2017-07-03 12:10 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 03/07/2017 04:08, Andreas Schwab wrote:
> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> On 29/06/2017 12:20, Andreas Schwab wrote:
>>> On Jun 29 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>>>
>>>> Since posix_spawn is not support to set errno in case of failure
>>>
>>> Is it?
>>
>> My understanding of posix_spawn return value [1] states that the error number
>> should be returned as the function return value, not on errno.
>
> But that doesn't forbid spurious writes to errno, does it?
My understanding is POSIX is not strictly regarding spurious errno writes,
but on general information [1] it has the rationale:
"In order to avoid this problem altogether for new functions, these
functions avoid using errno and, instead, return the error number directly
as the function return value; a return value of zero indicates that no
error was detected."
Also, making 'clone' not setting errno should also a QoI IMHO: this is an
Linux extension and making is avoid touching memory that might incur in
some issue should be less prone to errors.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-07-03 12:10 ` Adhemerval Zanella
@ 2017-07-03 13:02 ` Andreas Schwab
2017-07-03 13:32 ` Adhemerval Zanella
0 siblings, 1 reply; 15+ messages in thread
From: Andreas Schwab @ 2017-07-03 13:02 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
On Jul 03 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> My understanding is POSIX is not strictly regarding spurious errno writes,
> but on general information [1] it has the rationale:
>
> "In order to avoid this problem altogether for new functions, these
> functions avoid using errno and, instead, return the error number directly
> as the function return value; a return value of zero indicates that no
> error was detected."
It also notes (in section Signal Actions):
"Note in particular that even the "safe'' functions may modify the global
variable errno"
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] posix: Improve default posix_spawn implementation
2017-07-03 13:02 ` Andreas Schwab
@ 2017-07-03 13:32 ` Adhemerval Zanella
0 siblings, 0 replies; 15+ messages in thread
From: Adhemerval Zanella @ 2017-07-03 13:32 UTC (permalink / raw)
To: Andreas Schwab; +Cc: libc-alpha
On 03/07/2017 10:02, Andreas Schwab wrote:
> On Jul 03 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
>
>> My understanding is POSIX is not strictly regarding spurious errno writes,
>> but on general information [1] it has the rationale:
>>
>> "In order to avoid this problem altogether for new functions, these
>> functions avoid using errno and, instead, return the error number directly
>> as the function return value; a return value of zero indicates that no
>> error was detected."
>
> It also notes (in section Signal Actions):
>
> "Note in particular that even the "safe'' functions may modify the global
> variable errno"
So it is more an implementation detail and/or QoI to whether modify errno
on such functions and see no impeding reason of doing it on posix_spawn.
The only rationale I am not sure is if its worth to remove clone wrapper
error path setting errno.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-07-03 13:32 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-15 19:36 [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 2/3] posix: Improve default posix_spawn implementation Adhemerval Zanella
2017-06-28 20:27 ` Adhemerval Zanella
2017-06-29 14:22 ` Andreas Schwab
2017-06-29 15:01 ` Adhemerval Zanella
2017-06-29 15:20 ` Andreas Schwab
2017-06-29 17:39 ` Adhemerval Zanella
2017-07-03 7:08 ` Andreas Schwab
2017-07-03 12:10 ` Adhemerval Zanella
2017-07-03 13:02 ` Andreas Schwab
2017-07-03 13:32 ` Adhemerval Zanella
2017-05-15 19:36 ` [PATCH 3/3] posix: Refactor posix_spawn Adhemerval Zanella
2017-06-28 1:07 ` Rasmus Villemoes
2017-06-28 12:51 ` Adhemerval Zanella
2017-06-28 12:47 ` [PATCH 1/3] posix: Adapt tst-spawn{2,3} to use libsupport Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).