[PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD

[PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD

[наб]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

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(-)

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;
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD

[Jilles Tjoelker]

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.

-- 
Jilles Tjoelker

Re: [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD

[Herbert Xu]

Jilles Tjoelker <jilles@stack.nl> 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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available

[наб]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

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).
---
autoconf bits adapted almost-verbatim from the st_mtim check above

 configure.ac | 13 +++++++++++++
 src/redir.c  |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/configure.ac b/configure.ac
index 52aa429..b870a59 100644
--- a/configure.ac
+++ b/configure.ac
@@ -177,6 +177,19 @@ if test "$have_st_mtim" = "yes"; then
 		[Define if your `struct stat' has `st_mtim'])
 fi
 
+dnl F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
+AC_MSG_CHECKING(for F_DUPFD_CLOEXEC)
+AC_COMPILE_IFELSE(
+[AC_LANG_PROGRAM([#include <unistd.h>
+#include <fcntl.h>],
+[return fcntl(0, F_DUPFD_CLOEXEC, 0)])],
+have_dupfd_cloexec=yes, have_dupfd_cloexec=no)
+AC_MSG_RESULT($have_dupfd_cloexec)
+if test "$have_dupfd_cloexec" = "yes"; then
+	AC_DEFINE([HAVE_F_DUPFD_CLOEXEC], [1],
+		[Define if your system supports F_DUPFD_CLOEXEC])
+fi
+
 AC_ARG_WITH(libedit, AS_HELP_STRING(--with-libedit, [Compile with libedit support]))
 use_libedit=
 if test "$with_libedit" = "yes"; then
diff --git a/src/redir.c b/src/redir.c
index 5a5835c..c923bfa 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -446,14 +446,21 @@ savefd(int from, int ofd)
 	int newfd;
 	int err;
 
+#if HAVE_F_DUPFD_CLOEXEC
+	newfd = fcntl(from, F_DUPFD_CLOEXEC, 10);
+#else
 	newfd = fcntl(from, F_DUPFD, 10);
+#endif
+
 	err = newfd < 0 ? errno : 0;
 	if (err != EBADF) {
 		close(ofd);
 		if (err)
 			sh_error("%d: %s", from, strerror(err));
+#if !HAVE_F_DUPFD_CLOEXEC
 		else
 			fcntl(newfd, F_SETFD, FD_CLOEXEC);
+#endif
 	}
 
 	return newfd;
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available

[Herbert Xu]

наб <nabijaczleweli@nabijaczleweli.xyz> wrote:
> [-- text/plain, encoding quoted-printable, charset: us-ascii, 64 lines --]
> 
> 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).
> ---
> autoconf bits adapted almost-verbatim from the st_mtim check above
> 
> configure.ac | 13 +++++++++++++
> src/redir.c  |  7 +++++++
> 2 files changed, 20 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index 52aa429..b870a59 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -177,6 +177,19 @@ if test "$have_st_mtim" = "yes"; then
>                [Define if your `struct stat' has `st_mtim'])
> fi
> 
> +dnl F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
> +AC_MSG_CHECKING(for F_DUPFD_CLOEXEC)
> +AC_COMPILE_IFELSE(
> +[AC_LANG_PROGRAM([#include <unistd.h>
> +#include <fcntl.h>],
> +[return fcntl(0, F_DUPFD_CLOEXEC, 0)])],
> +have_dupfd_cloexec=yes, have_dupfd_cloexec=no)
> +AC_MSG_RESULT($have_dupfd_cloexec)
> +if test "$have_dupfd_cloexec" = "yes"; then
> +       AC_DEFINE([HAVE_F_DUPFD_CLOEXEC], [1],
> +               [Define if your system supports F_DUPFD_CLOEXEC])
> +fi

Could you please define this symbol always? That is,

...
have_dupfd_cloexec=1, have_dupfd_cloexec=0)
AC_DEFINE([HAVE_F_DUPFD_CLOEXEC], [${have_dupfd_cloexec}],
	  [Set to 1 if your system supports F_DUPFD_CLOEXEC])

>        err = newfd < 0 ? errno : 0;
>        if (err != EBADF) {
>                close(ofd);
>                if (err)
>                        sh_error("%d: %s", from, strerror(err));
> +#if !HAVE_F_DUPFD_CLOEXEC
>                else
>                        fcntl(newfd, F_SETFD, FD_CLOEXEC);
> +#endif

Then we could change this to

		else if (!HAVE_F_DUPFD_CLOEXEC)
			fcntl(newfd, F_SETFD, FD_CLOEXEC);

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available

[наб]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]

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).
---
 configure.ac | 11 +++++++++++
 src/redir.c  |  7 ++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 52aa429..5524650 100644
--- a/configure.ac
+++ b/configure.ac
@@ -177,6 +177,17 @@ if test "$have_st_mtim" = "yes"; then
 		[Define if your `struct stat' has `st_mtim'])
 fi
 
+dnl F_DUPFD_CLOEXEC is a mandatory part of POSIX since Issue 7
+AC_MSG_CHECKING(for F_DUPFD_CLOEXEC)
+AC_COMPILE_IFELSE(
+[AC_LANG_PROGRAM([#include <unistd.h>
+#include <fcntl.h>],
+[return fcntl(0, F_DUPFD_CLOEXEC, 0)])],
+have_dupfd_cloexec=1, have_dupfd_cloexec=0)
+AC_MSG_RESULT($(expr yes \& $have_dupfd_cloexec \| no))
+AC_DEFINE_UNQUOTED([HAVE_F_DUPFD_CLOEXEC], [$have_dupfd_cloexec],
+	[Define to 1 your system supports F_DUPFD_CLOEXEC])
+
 AC_ARG_WITH(libedit, AS_HELP_STRING(--with-libedit, [Compile with libedit support]))
 use_libedit=
 if test "$with_libedit" = "yes"; then
diff --git a/src/redir.c b/src/redir.c
index 5a5835c..631ddc9 100644
--- a/src/redir.c
+++ b/src/redir.c
@@ -446,13 +446,18 @@ savefd(int from, int ofd)
 	int newfd;
 	int err;
 
+#if HAVE_F_DUPFD_CLOEXEC
+	newfd = fcntl(from, F_DUPFD_CLOEXEC, 10);
+#else
 	newfd = fcntl(from, F_DUPFD, 10);
+#endif
+
 	err = newfd < 0 ? errno : 0;
 	if (err != EBADF) {
 		close(ofd);
 		if (err)
 			sh_error("%d: %s", from, strerror(err));
-		else
+		else if(!HAVE_F_DUPFD_CLOEXEC)
 			fcntl(newfd, F_SETFD, FD_CLOEXEC);
 	}
 
-- 
2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available

[наб]

[-- Attachment #1: Type: text/plain, Size: 81 bytes --]

(this is v3 with your comments applied, naturally; my finger slipped)

наб

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available

[Herbert Xu]

On Thu, Jan 05, 2023 at 01:43:21PM +0100, наб wrote:
> 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).
> ---
>  configure.ac | 11 +++++++++++
>  src/redir.c  |  7 ++++++-
>  2 files changed, 17 insertions(+), 1 deletion(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt