posix_spawn(3): avoid redundant fcntl(2) calls

[kevans]

Summary

Descriptors provisioned by dup2 already have close-on-exec cleared, so we don't do need to do it in any case where we're operating on such a descriptor.

We also don't need to let O_CLOEXEC stay in fae_oflag if we're just going to clear it later, since it indeed makes little sense to open any file O_CLOEXEC here.

Finally, we can circumvent the dup2() in dup2 fileactions if oldfd == newfd and just clear close-on-exec, since that wouldn't have happened if we just called dup2(old, new).

[jilles]

The dup2 part looks good.

Regarding the open part, https://www.austingroupbugs.net/view.php?id=1208#c4111 suggests that O_CLOEXEC for posix_spawn_file_actions_addopen() may be useful with new actions that use file descriptors, although it does not really provide an immediate use case (instead of opening a directory for use with posix_spawn_file_actions_fchdir(), it makes more sense to use posix_spawn_file_actions_chdir()).

Unrelated to this review, the extensions proposed in that bug seem useful for allowing posix_spawnp() use in find.

[kevans]

In D25033#551509, @jilles wrote:

The dup2 part looks good.

Regarding the open part, https://www.austingroupbugs.net/view.php?id=1208#c4111 suggests that O_CLOEXEC for posix_spawn_file_actions_addopen() may be useful with new actions that use file descriptors, although it does not really provide an immediate use case (instead of opening a directory for use with posix_spawn_file_actions_fchdir(), it makes more sense to use posix_spawn_file_actions_chdir()).

Heh, this one's caused me to do a little bit of soul searching. :-) Not adding _addchdir because you can _addopen and _addfchdir would be absolutely maddening to me. That said, the spec doesn't specifically forbid O_CLOEXEC, and testing shows that other implementations are hit-and-miss as to whether they honor it or not. If we decide that we should be honoring O_CLOEXEC, I suspect we should actually be going a step further and explicitly set FD_CLOEXEC if we had to dup2 it over. It's somewhat tempting to then use F_DUP2FD/F_DUP2FD_CLOEXEC based on the presence of O_CLOEXEC instead of dup2'ing it in the first place.

The FAE_DUP2 case I find particularly interesting. The spec doesn't seem to suggest any particular behavior for old == new, and in fact goes as far as to say that it's just identical to dup2. However, general implementation consensus seems to be that old == new clears FD_CLOEXEC which is directly contradictory to the spec, which mandates that dup2 does not touch FD_CLOEXEC for old == new.

[jilles]

In D25033#551601, @kevans wrote:

In D25033#551509, @jilles wrote:

The dup2 part looks good.

Regarding the open part, https://www.austingroupbugs.net/view.php?id=1208#c4111 suggests that O_CLOEXEC for posix_spawn_file_actions_addopen() may be useful with new actions that use file descriptors, although it does not really provide an immediate use case (instead of opening a directory for use with posix_spawn_file_actions_fchdir(), it makes more sense to use posix_spawn_file_actions_chdir()).

Heh, this one's caused me to do a little bit of soul searching. :-) Not adding _addchdir because you can _addopen and _addfchdir would be absolutely maddening to me. That said, the spec doesn't specifically forbid O_CLOEXEC, and testing shows that other implementations are hit-and-miss as to whether they honor it or not. If we decide that we should be honoring O_CLOEXEC, I suspect we should actually be going a step further and explicitly set FD_CLOEXEC if we had to dup2 it over. It's somewhat tempting to then use F_DUP2FD/F_DUP2FD_CLOEXEC based on the presence of O_CLOEXEC instead of dup2'ing it in the first place.

dup3() would certainly be better than dup2(). Note that https://www.austingroupbugs.net/view.php?id=411 contains dup3() but not F_DUP2FD_CLOEXEC.

The FAE_DUP2 case I find particularly interesting. The spec doesn't seem to suggest any particular behavior for old == new, and in fact goes as far as to say that it's just identical to dup2. However, general implementation consensus seems to be that old == new clears FD_CLOEXEC which is directly contradictory to the spec, which mandates that dup2 does not touch FD_CLOEXEC for old == new.

This is a change requested in https://www.austingroupbugs.net/view.php?id=411 .

[kevans]

In D25033#551706, @jilles wrote:

In D25033#551601, @kevans wrote:

In D25033#551509, @jilles wrote:

The dup2 part looks good.

Regarding the open part, https://www.austingroupbugs.net/view.php?id=1208#c4111 suggests that O_CLOEXEC for posix_spawn_file_actions_addopen() may be useful with new actions that use file descriptors, although it does not really provide an immediate use case (instead of opening a directory for use with posix_spawn_file_actions_fchdir(), it makes more sense to use posix_spawn_file_actions_chdir()).

Heh, this one's caused me to do a little bit of soul searching. :-) Not adding _addchdir because you can _addopen and _addfchdir would be absolutely maddening to me. That said, the spec doesn't specifically forbid O_CLOEXEC, and testing shows that other implementations are hit-and-miss as to whether they honor it or not. If we decide that we should be honoring O_CLOEXEC, I suspect we should actually be going a step further and explicitly set FD_CLOEXEC if we had to dup2 it over. It's somewhat tempting to then use F_DUP2FD/F_DUP2FD_CLOEXEC based on the presence of O_CLOEXEC instead of dup2'ing it in the first place.

dup3() would certainly be better than dup2(). Note that https://www.austingroupbugs.net/view.php?id=411 contains dup3() but not F_DUP2FD_CLOEXEC.

Sure- more than happy to angle towards dup3() given that it has a chance of getting incorporated into future specs.