Message ID | ed432eaf-2139-6fde-51ba-b85aa06a6ac3@linaro.org |
---|---|
State | New |
Headers | show |
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.
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/
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.
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
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.
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.
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]);