* [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD
@ 2022-12-16 23:01 наб
2022-12-17 14:44 ` Jilles Tjoelker
0 siblings, 1 reply; 8+ messages in thread
From: наб @ 2022-12-16 23:01 UTC (permalink / raw
To: dash
[-- 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 --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD
2022-12-16 23:01 [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD наб
@ 2022-12-17 14:44 ` Jilles Tjoelker
2022-12-18 2:29 ` Herbert Xu
0 siblings, 1 reply; 8+ messages in thread
From: Jilles Tjoelker @ 2022-12-17 14:44 UTC (permalink / raw
To: наб; +Cc: dash
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD
2022-12-17 14:44 ` Jilles Tjoelker
@ 2022-12-18 2:29 ` Herbert Xu
2022-12-18 2:59 ` [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available наб
0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2022-12-18 2:29 UTC (permalink / raw
To: Jilles Tjoelker; +Cc: nabijaczleweli, dash
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available
2022-12-18 2:29 ` Herbert Xu
@ 2022-12-18 2:59 ` наб
2023-01-05 9:17 ` Herbert Xu
0 siblings, 1 reply; 8+ messages in thread
From: наб @ 2022-12-18 2:59 UTC (permalink / raw
To: Herbert Xu; +Cc: Jilles Tjoelker, dash
[-- 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 --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available
2022-12-18 2:59 ` [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available наб
@ 2023-01-05 9:17 ` Herbert Xu
2023-01-05 12:43 ` наб
0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2023-01-05 9:17 UTC (permalink / raw
To: наб; +Cc: jilles, dash
наб <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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available
2023-01-05 9:17 ` Herbert Xu
@ 2023-01-05 12:43 ` наб
2023-01-05 12:45 ` наб
2023-01-08 12:06 ` Herbert Xu
0 siblings, 2 replies; 8+ messages in thread
From: наб @ 2023-01-05 12:43 UTC (permalink / raw
To: Herbert Xu; +Cc: jilles, dash
[-- 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 --]
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available
2023-01-05 12:43 ` наб
@ 2023-01-05 12:45 ` наб
2023-01-08 12:06 ` Herbert Xu
1 sibling, 0 replies; 8+ messages in thread
From: наб @ 2023-01-05 12:45 UTC (permalink / raw
To: Herbert Xu; +Cc: jilles, dash
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available
2023-01-05 12:43 ` наб
2023-01-05 12:45 ` наб
@ 2023-01-08 12:06 ` Herbert Xu
1 sibling, 0 replies; 8+ messages in thread
From: Herbert Xu @ 2023-01-08 12:06 UTC (permalink / raw
To: наб; +Cc: jilles, dash
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-01-08 12:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-16 23:01 [PATCH] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD наб
2022-12-17 14:44 ` Jilles Tjoelker
2022-12-18 2:29 ` Herbert Xu
2022-12-18 2:59 ` [PATCH v2] redir: savefd: use F_DUPFD_CLOEXEC instead of F_DUPFD+F_SETFD, if available наб
2023-01-05 9:17 ` Herbert Xu
2023-01-05 12:43 ` наб
2023-01-05 12:45 ` наб
2023-01-08 12:06 ` Herbert Xu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).