redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD

Commit Message

Ahelenia Ziemiańska Dec. 16, 2022, 11:01 p.m. UTC

This saves a syscall on every source file open, &c.;
F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
(Austin Group Interpretation 1003.1-2001 #171).
---
 src/redir.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

Jilles Tjoelker Dec. 17, 2022, 2:44 p.m. UTC | #1

On Sat, Dec 17, 2022 at 12:01:09AM +0100, наб wrote:
> [use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD]
> This saves a syscall on every source file open, &c.;
> F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
> (Austin Group Interpretation 1003.1-2001 #171).

This is a small optimization and simplification if all target platforms
support F_DUPFD_CLOEXEC, but it may not be worth it if this code is
supposed to work on older platforms. In the mailing list archives, there
are various patches (some applied) to make dash work on old platforms.

Often, the newer _CLOEXEC interfaces are used to avoid race conditions,
but not here, which might confuse porters that the necessary changes are
more complicated than they need to be.

Herbert Xu Dec. 18, 2022, 2:29 a.m. UTC | #2

Jilles Tjoelker <[e-mail]> wrote:
> On Sat, Dec 17, 2022 at 12:01:09AM +0100, наб wrote:
>> [use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD]
>> This saves a syscall on every source file open, &c.;
>> F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
>> (Austin Group Interpretation 1003.1-2001 #171).
> 
> This is a small optimization and simplification if all target platforms
> support F_DUPFD_CLOEXEC, but it may not be worth it if this code is
> supposed to work on older platforms. In the mailing list archives, there
> are various patches (some applied) to make dash work on old platforms.
> 
> Often, the newer _CLOEXEC interfaces are used to avoid race conditions,
> but not here, which might confuse porters that the necessary changes are
> more complicated than they need to be.

Right.  I would accept this patch if you add the autoconf bits
to retain support for older platforms.

Thanks,

Patch

diff --git a/src/redir.c b/src/redir.c
index 5a5835c..9fee74f 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -446,14 +446,12 @@  savefd(int from, int ofd)
 	int newfd;
 	int err;
 
-	newfd = fcntl(from, F_DUPFD, 10);
+	newfd = fcntl(from, F_DUPFD_CLOEXEC, 10);
 	err = newfd < 0 ? errno : 0;
 	if (err != EBADF) {
 		close(ofd);
 		if (err)
 			sh_error("%d: %s", from, strerror(err));
-		else
-			fcntl(newfd, F_SETFD, FD_CLOEXEC);
 	}
 
 	return newfd;