| Message ID | 20170413192548.BF5A040EBCF62@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
LGTM with with one remark below. On 13/04/2017 16:25, Florian Weimer wrote: > 2017-04-13 Florian Weimer <fweimer@redhat.com> > > * include/unistd.h (__have_dup3): Remove declaration. > * libio/freopen.c (freopen): Assume that O_CLOEXEC is defined and > dup3 is available. > * libio/freopen64.c (freopen64): Likewise. > * socket/Makefile (aux): Remove have_sock_cloexec. > * socket/have_sock_cloexec.c: Remove file. > * sysdeps/mach/hurd/kernel-features.h (__ASSUME_DUP3): Remove > definition. > * sysdeps/unix/sysv/linux/kernel-features.h (__ASSUME_DUP3): > Likewise. > * sysdeps/nacl/kernel-features.h: Update comment. > > diff --git a/include/unistd.h b/include/unistd.h > index e15fa0e..16a8815 100644 > --- a/include/unistd.h > +++ b/include/unistd.h > @@ -171,8 +171,6 @@ extern int __libc_pause (void); > /* Not cancelable variant. */ > extern int __pause_nocancel (void) attribute_hidden; > > -extern int __have_dup3 attribute_hidden; > - > extern int __getlogin_r_loginuid (char *name, size_t namesize) > attribute_hidden; > > diff --git a/libio/freopen.c b/libio/freopen.c > index 03e3ae7..ad1c848 100644 > --- a/libio/freopen.c > +++ b/libio/freopen.c > @@ -78,32 +78,9 @@ freopen (const char *filename, const char *mode, FILE *fp) > > if (fd != -1) > { > -#ifdef O_CLOEXEC > -# ifndef __ASSUME_DUP3 > - int newfd; > - if (__have_dup3 < 0) > - newfd = -1; > - else > - newfd = > -# endif > - __dup3 (_IO_fileno (result), fd, > - (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 > - ? O_CLOEXEC : 0); > -#else > -# define newfd 1 > -#endif > - > -#ifndef __ASSUME_DUP3 > - if (newfd < 0) > - { > - if (errno == ENOSYS) > - __have_dup3 = -1; > - > - __dup2 (_IO_fileno (result), fd); > - if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0) > - __fcntl (fd, F_SETFD, FD_CLOEXEC); > - } > -#endif > + __dup3 (_IO_fileno (result), fd, > + (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 > + ? O_CLOEXEC : 0); Shouldn't we check the result of __dup3 and act accordingly in this situation (basically EINTR and EMFILE)? > __close (_IO_fileno (result)); > _IO_fileno (result) = fd; > } > diff --git a/libio/freopen64.c b/libio/freopen64.c > index 1f0d8ab..adf749a 100644 > --- a/libio/freopen64.c > +++ b/libio/freopen64.c > @@ -61,32 +61,9 @@ freopen64 (const char *filename, const char *mode, FILE *fp) > > if (fd != -1) > { > -#ifdef O_CLOEXEC > -# ifndef __ASSUME_DUP3 > - int newfd; > - if (__have_dup3 < 0) > - newfd = -1; > - else > - newfd = > -# endif > - __dup3 (_IO_fileno (result), fd, > - (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 > - ? O_CLOEXEC : 0); > -#else > -# define newfd 1 > -#endif > - > -#ifndef __ASSUME_DUP3 > - if (newfd < 0) > - { > - if (errno == ENOSYS) > - __have_dup3 = -1; > - > - __dup2 (_IO_fileno (result), fd); > - if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0) > - __fcntl (fd, F_SETFD, FD_CLOEXEC); > - } > -#endif > + __dup3 (_IO_fileno (result), fd, > + (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 > + ? O_CLOEXEC : 0); Same as before. > __close (_IO_fileno (result)); > _IO_fileno (result) = fd; > } > diff --git a/socket/Makefile b/socket/Makefile > index c299d34..25d4f68 100644 > --- a/socket/Makefile > +++ b/socket/Makefile > @@ -31,6 +31,6 @@ routines := accept bind connect getpeername getsockname getsockopt \ > setsockopt shutdown socket socketpair isfdtype opensock \ > sockatmark accept4 recvmmsg sendmmsg > > -aux := have_sock_cloexec sa_len > +aux := sa_len > > include ../Rules > diff --git a/socket/have_sock_cloexec.c b/socket/have_sock_cloexec.c > deleted file mode 100644 > index 579577d..0000000 > --- a/socket/have_sock_cloexec.c > +++ /dev/null > @@ -1,24 +0,0 @@ > -/* Copyright (C) 2008-2017 Free Software Foundation, Inc. > - This file is part of the GNU C Library. > - > - The GNU C Library is free software; you can redistribute it and/or > - modify it under the terms of the GNU Lesser General Public > - License as published by the Free Software Foundation; either > - version 2.1 of the License, or (at your option) any later version. > - > - The GNU C Library is distributed in the hope that it will be useful, > - but WITHOUT ANY WARRANTY; without even the implied warranty of > - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > - Lesser General Public License for more details. > - > - You should have received a copy of the GNU Lesser General Public > - License along with the GNU C Library; if not, see > - <http://www.gnu.org/licenses/>. */ > - > -#include <fcntl.h> > -#include <sys/socket.h> > -#include <kernel-features.h> > - > -#if defined O_CLOEXEC && !defined __ASSUME_DUP3 > -int __have_dup3; > -#endif > diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h > index 687c7f0..60c5bca 100644 > --- a/sysdeps/mach/hurd/kernel-features.h > +++ b/sysdeps/mach/hurd/kernel-features.h > @@ -21,5 +21,4 @@ > But those referring to POSIX-level features like O_* flags can be. */ > > #define __ASSUME_O_CLOEXEC 1 > -#define __ASSUME_DUP3 1 > #define __ASSUME_ACCEPT4 1 > diff --git a/sysdeps/nacl/kernel-features.h b/sysdeps/nacl/kernel-features.h > index 38a47f1..4a5808b 100644 > --- a/sysdeps/nacl/kernel-features.h > +++ b/sysdeps/nacl/kernel-features.h > @@ -23,6 +23,5 @@ > #define __ASSUME_O_CLOEXEC 1 > > /* > -#define __ASSUME_DUP3 1 > #define __ASSUME_ACCEPT4 1 > */ > diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h > index 233e302..e0eb4e2 100644 > --- a/sysdeps/unix/sysv/linux/kernel-features.h > +++ b/sysdeps/unix/sysv/linux/kernel-features.h > @@ -74,7 +74,6 @@ > /* Support for various CLOEXEC and NONBLOCK flags was added in > 2.6.27. */ > #define __ASSUME_IN_NONBLOCK 1 > -#define __ASSUME_DUP3 1 > > /* Support for accept4 functionality was added in 2.6.28, but for some > architectures using a separate syscall rather than socketcall that >
On 04/17/2017 08:16 PM, Adhemerval Zanella wrote: > Shouldn't we check the result of __dup3 and act accordingly in this situation > (basically EINTR and EMFILE)? Yes, but that's an existing bug in the code. I would like to keep this change the bug fix separate. I filed bug 21393 for the missing error check. Thanks, Florian
On Apr 17 2017, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: >> + __dup3 (_IO_fileno (result), fd, >> + (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 >> + ? O_CLOEXEC : 0); > > Shouldn't we check the result of __dup3 and act accordingly in this situation > (basically EINTR and EMFILE)? I don't think either is possible, since both descriptors are already allocated. Andreas.
diff --git a/include/unistd.h b/include/unistd.h index e15fa0e..16a8815 100644 --- a/include/unistd.h +++ b/include/unistd.h @@ -171,8 +171,6 @@ extern int __libc_pause (void); /* Not cancelable variant. */ extern int __pause_nocancel (void) attribute_hidden; -extern int __have_dup3 attribute_hidden; - extern int __getlogin_r_loginuid (char *name, size_t namesize) attribute_hidden; diff --git a/libio/freopen.c b/libio/freopen.c index 03e3ae7..ad1c848 100644 --- a/libio/freopen.c +++ b/libio/freopen.c @@ -78,32 +78,9 @@ freopen (const char *filename, const char *mode, FILE *fp) if (fd != -1) { -#ifdef O_CLOEXEC -# ifndef __ASSUME_DUP3 - int newfd; - if (__have_dup3 < 0) - newfd = -1; - else - newfd = -# endif - __dup3 (_IO_fileno (result), fd, - (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 - ? O_CLOEXEC : 0); -#else -# define newfd 1 -#endif - -#ifndef __ASSUME_DUP3 - if (newfd < 0) - { - if (errno == ENOSYS) - __have_dup3 = -1; - - __dup2 (_IO_fileno (result), fd); - if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0) - __fcntl (fd, F_SETFD, FD_CLOEXEC); - } -#endif + __dup3 (_IO_fileno (result), fd, + (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 + ? O_CLOEXEC : 0); __close (_IO_fileno (result)); _IO_fileno (result) = fd; } diff --git a/libio/freopen64.c b/libio/freopen64.c index 1f0d8ab..adf749a 100644 --- a/libio/freopen64.c +++ b/libio/freopen64.c @@ -61,32 +61,9 @@ freopen64 (const char *filename, const char *mode, FILE *fp) if (fd != -1) { -#ifdef O_CLOEXEC -# ifndef __ASSUME_DUP3 - int newfd; - if (__have_dup3 < 0) - newfd = -1; - else - newfd = -# endif - __dup3 (_IO_fileno (result), fd, - (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 - ? O_CLOEXEC : 0); -#else -# define newfd 1 -#endif - -#ifndef __ASSUME_DUP3 - if (newfd < 0) - { - if (errno == ENOSYS) - __have_dup3 = -1; - - __dup2 (_IO_fileno (result), fd); - if ((result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0) - __fcntl (fd, F_SETFD, FD_CLOEXEC); - } -#endif + __dup3 (_IO_fileno (result), fd, + (result->_flags2 & _IO_FLAGS2_CLOEXEC) != 0 + ? O_CLOEXEC : 0); __close (_IO_fileno (result)); _IO_fileno (result) = fd; } diff --git a/socket/Makefile b/socket/Makefile index c299d34..25d4f68 100644 --- a/socket/Makefile +++ b/socket/Makefile @@ -31,6 +31,6 @@ routines := accept bind connect getpeername getsockname getsockopt \ setsockopt shutdown socket socketpair isfdtype opensock \ sockatmark accept4 recvmmsg sendmmsg -aux := have_sock_cloexec sa_len +aux := sa_len include ../Rules diff --git a/socket/have_sock_cloexec.c b/socket/have_sock_cloexec.c deleted file mode 100644 index 579577d..0000000 --- a/socket/have_sock_cloexec.c +++ /dev/null @@ -1,24 +0,0 @@ -/* Copyright (C) 2008-2017 Free Software Foundation, Inc. - This file is part of the GNU C Library. - - The GNU C Library is free software; you can redistribute it and/or - modify it under the terms of the GNU Lesser General Public - License as published by the Free Software Foundation; either - version 2.1 of the License, or (at your option) any later version. - - The GNU C Library is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - Lesser General Public License for more details. - - You should have received a copy of the GNU Lesser General Public - License along with the GNU C Library; if not, see - <http://www.gnu.org/licenses/>. */ - -#include <fcntl.h> -#include <sys/socket.h> -#include <kernel-features.h> - -#if defined O_CLOEXEC && !defined __ASSUME_DUP3 -int __have_dup3; -#endif diff --git a/sysdeps/mach/hurd/kernel-features.h b/sysdeps/mach/hurd/kernel-features.h index 687c7f0..60c5bca 100644 --- a/sysdeps/mach/hurd/kernel-features.h +++ b/sysdeps/mach/hurd/kernel-features.h @@ -21,5 +21,4 @@ But those referring to POSIX-level features like O_* flags can be. */ #define __ASSUME_O_CLOEXEC 1 -#define __ASSUME_DUP3 1 #define __ASSUME_ACCEPT4 1 diff --git a/sysdeps/nacl/kernel-features.h b/sysdeps/nacl/kernel-features.h index 38a47f1..4a5808b 100644 --- a/sysdeps/nacl/kernel-features.h +++ b/sysdeps/nacl/kernel-features.h @@ -23,6 +23,5 @@ #define __ASSUME_O_CLOEXEC 1 /* -#define __ASSUME_DUP3 1 #define __ASSUME_ACCEPT4 1 */ diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h index 233e302..e0eb4e2 100644 --- a/sysdeps/unix/sysv/linux/kernel-features.h +++ b/sysdeps/unix/sysv/linux/kernel-features.h @@ -74,7 +74,6 @@ /* Support for various CLOEXEC and NONBLOCK flags was added in 2.6.27. */ #define __ASSUME_IN_NONBLOCK 1 -#define __ASSUME_DUP3 1 /* Support for accept4 functionality was added in 2.6.28, but for some architectures using a separate syscall rather than socketcall that