[PATCH v2 3/3] posix: New Linux posix_spawn{p} implementation
Adhemerval Zanella
adhemerval.zanella@linaro.org
Wed Sep 14 19:37:00 GMT 2016
On 31/08/2016 18:13, Rasmus Villemoes wrote:
>> + switch (action->tag)
>> + {
>> + case spawn_do_close:
>> + if ((ret =
>> + 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;
>> + }
>
> I may have missed it in the original discussion, but what is the
> rationale for this? POSIX says
>
> If the file_actions argument is not NULL, and specifies any close,
> dup2, or open actions to be performed, and if posix_spawn() or
> posix_spawnp() fails for any of the reasons that would cause close(),
> dup2(), or open() to fail, an error value shall be returned as
> described by close(), dup2(), and open(), respectively
I haven't joined the original discussion also (if any) for old implementation
behaviour, but I think this conforms to austin group issue #370 [1] where it
changed:
fails for any of the reasons that would cause close( ), dup2( ), or open( ) to
fail, an error value shall be returned
to:
fails for any of the reasons that would cause close( ), dup2( ), or open( ) to
fail, other than attempting a close( ) on a file descriptor that is in range
but already closed, an error value shall be returned
The documentation at [2] seems to not have this updates issues description.
[1] http://austingroupbugs.net/view.php?id=370
[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn.html
>
>> + break;
>> +
>> + case spawn_do_open:
>> + {
>> + 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 ((ret = __dup2 (new_fd, action->action.open_action.fd))
>> + != action->action.open_action.fd)
>> + goto fail;
>> +
>> + if ((ret = close_not_cancel (new_fd)) != 0)
>> + goto fail;
>> + }
>> + }
>
> This is also how I'd have implemented it, but POSIX explicitly says
>
> If fildes was already an open file descriptor, it shall be closed
> before the new file is opened.
>
> Some, if slightly pathological, examples of how the difference could be
> observed:
>
> * ENFILE/EMFILE
I think this should be an issue iff the program tries call posix_spawn
with a total number of file descriptors equal to RLIMIT_NOFILE.
>
> * Some single-open special device; following POSIX,
> 'action_open("/dev/watchdog", 47); action_open("/dev/watchdog", 47);'
> should both succeed if the first does, whereas the second will fail
> with the current code.
Right, this should more problematic to handle. I think an option is to
add a 'fcntl(fd, F_GETFD) != -1 || errno != EBADF' check before the open
syscall and close the file descriptor in this case.
More information about the Libc-alpha
mailing list