diff mbox series

Move is_valid_fd to filedescriptor.c file.

Message ID dc25d696-aa99-ee09-d6bb-be0c74e489fe@suse.cz
State New
Headers show
Series Move is_valid_fd to filedescriptor.c file. | expand

Commit Message

Martin Liška Aug. 9, 2019, 7:15 a.m. UTC
Hi.

As Jakub correctly noted, I used a piggy backing to put the new function
to a file that is supposed to contain different functions. So that
I'm suggesting a new file. Moreover, I'm also adding dup2 fallback.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

libiberty/ChangeLog:

2019-08-08  Martin Liska  <mliska@suse.cz>

	* Makefile.in: Add filedescriptor.c.
	* filedescriptor.c: New file.
	* lrealpath.c (is_valid_fd): Remove.
---
 libiberty/Makefile.in      | 14 +++++++++++-
 libiberty/filedescriptor.c | 47 ++++++++++++++++++++++++++++++++++++++
 libiberty/lrealpath.c      | 16 -------------
 3 files changed, 60 insertions(+), 17 deletions(-)
 create mode 100644 libiberty/filedescriptor.c

Comments

Ian Lance Taylor Aug. 9, 2019, 6:05 p.m. UTC | #1
On Fri, Aug 9, 2019 at 12:15 AM Martin Liška <mliska@suse.cz> wrote:
>
> As Jakub correctly noted, I used a piggy backing to put the new function
> to a file that is supposed to contain different functions. So that
> I'm suggesting a new file. Moreover, I'm also adding dup2 fallback.
>
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
>
> Ready to be installed?
> Thanks,
> Martin
>
> libiberty/ChangeLog:
>
> 2019-08-08  Martin Liska  <mliska@suse.cz>
>
>         * Makefile.in: Add filedescriptor.c.
>         * filedescriptor.c: New file.
>         * lrealpath.c (is_valid_fd): Remove.


I don't understand the dup2 fallback.  It looks backward: if dup2(fd,
fd) will return -1 if fd does not exist.

I also don't think it's needed.  fcntl(fd, F_GETFD) should work on all
Unix systems.  It should certainly work on all Unix systems that have
dup2.  What systems are you concerned about?

Ian
Jakub Jelinek Aug. 9, 2019, 6:13 p.m. UTC | #2
On Fri, Aug 09, 2019 at 11:05:42AM -0700, Ian Lance Taylor wrote:
> >         * Makefile.in: Add filedescriptor.c.
> >         * filedescriptor.c: New file.
> >         * lrealpath.c (is_valid_fd): Remove.
> 
> 
> I don't understand the dup2 fallback.  It looks backward: if dup2(fd,
> fd) will return -1 if fd does not exist.

Sure, it should be >= 0 instead of < 0.

> I also don't think it's needed.  fcntl(fd, F_GETFD) should work on all
> Unix systems.  It should certainly work on all Unix systems that have
> dup2.  What systems are you concerned about?

That was just my suggestion based on what gnulib does:
http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fcntl.c
I thought they had a reason but maybe they don't.
It was added in
http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/fcntl.c?id=021c8619190757f535c72ad5cdb1d624e19620d6

	Jakub
Ian Lance Taylor Aug. 9, 2019, 6:16 p.m. UTC | #3
On Fri, Aug 9, 2019 at 11:13 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Aug 09, 2019 at 11:05:42AM -0700, Ian Lance Taylor wrote:
> > >         * Makefile.in: Add filedescriptor.c.
> > >         * filedescriptor.c: New file.
> > >         * lrealpath.c (is_valid_fd): Remove.
> >
> >
> > I don't understand the dup2 fallback.  It looks backward: if dup2(fd,
> > fd) will return -1 if fd does not exist.
>
> Sure, it should be >= 0 instead of < 0.
>
> > I also don't think it's needed.  fcntl(fd, F_GETFD) should work on all
> > Unix systems.  It should certainly work on all Unix systems that have
> > dup2.  What systems are you concerned about?
>
> That was just my suggestion based on what gnulib does:
> http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/fcntl.c
> I thought they had a reason but maybe they don't.
> It was added in
> http://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/fcntl.c?id=021c8619190757f535c72ad5cdb1d624e19620d6

Well, if it's need for Mingw it's fine with me, fixed as you suggest above.

Ian
diff mbox series

Patch

diff --git a/libiberty/Makefile.in b/libiberty/Makefile.in
index 0be45b4ae8e..f1628d4ee0d 100644
--- a/libiberty/Makefile.in
+++ b/libiberty/Makefile.in
@@ -127,7 +127,7 @@  CFILES = alloca.c argv.c asprintf.c atexit.c				\
 	calloc.c choose-temp.c clock.c concat.c cp-demangle.c		\
 	 cp-demint.c cplus-dem.c crc32.c				\
 	d-demangle.c dwarfnames.c dyn-string.c				\
-	fdmatch.c ffs.c fibheap.c filename_cmp.c floatformat.c		\
+	fdmatch.c ffs.c fibheap.c filedescriptor.c filename_cmp.c floatformat.c		\
 	fnmatch.c fopen_unlocked.c					\
 	getcwd.c getopt.c getopt1.c getpagesize.c getpwd.c getruntime.c	\
          gettimeofday.c                                                 \
@@ -171,6 +171,7 @@  REQUIRED_OFILES =							\
 	./cp-demint.$(objext) ./crc32.$(objext) ./d-demangle.$(objext)	\
 	./dwarfnames.$(objext) ./dyn-string.$(objext)			\
 	./fdmatch.$(objext) ./fibheap.$(objext)				\
+	./filedescriptor.$(objext)	\
 	./filename_cmp.$(objext) ./floatformat.$(objext)		\
 	./fnmatch.$(objext) ./fopen_unlocked.$(objext)			\
 	./getopt.$(objext) ./getopt1.$(objext) ./getpwd.$(objext)	\
@@ -756,6 +757,17 @@  $(CONFIGURED_OFILES): stamp-picdir stamp-noasandir
 	else true; fi
 	$(COMPILE.c) $(srcdir)/fibheap.c $(OUTPUT_OPTION)
 
+./filedescriptor.$(objext): $(srcdir)/filedescriptor.c config.h $(INCDIR)/ansidecl.h \
+	$(INCDIR)/libiberty.h
+	if [ x"$(PICFLAG)" != x ]; then \
+	  $(COMPILE.c) $(PICFLAG) $(srcdir)/filedescriptor.c -o pic/$@; \
+	else true; fi
+	if [ x"$(NOASANFLAG)" != x ]; then \
+	  $(COMPILE.c) $(PICFLAG) $(NOASANFLAG) $(srcdir)/filedescriptor.c -o noasan/$@; \
+	else true; fi
+	$(COMPILE.c) $(srcdir)/filedescriptor.c $(OUTPUT_OPTION)
+
+
 ./filename_cmp.$(objext): $(srcdir)/filename_cmp.c config.h $(INCDIR)/ansidecl.h \
 	$(INCDIR)/filenames.h $(INCDIR)/hashtab.h \
 	$(INCDIR)/safe-ctype.h
diff --git a/libiberty/filedescriptor.c b/libiberty/filedescriptor.c
new file mode 100644
index 00000000000..3a1a68d1eef
--- /dev/null
+++ b/libiberty/filedescriptor.c
@@ -0,0 +1,47 @@ 
+/* File descriptor related functions.
+
+   Copyright (C) 2019 Free Software Foundation, Inc.
+
+   This file is part of the libiberty library.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2 of the License, or
+   (at your option) any later version.
+
+   This program 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 General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software
+   Foundation, Inc., 51 Franklin Street - Fifth Floor,
+   Boston, MA 02110-1301, USA.  */
+
+#include "config.h"
+#include "ansidecl.h"
+#include "libiberty.h"
+
+#ifdef HAVE_FCNTL_H
+#include <fcntl.h>
+#endif
+
+#if defined (_WIN32)
+#define WIN32_LEAN_AND_MEAN
+#include <windows.h> /* for GetFullPathName */
+#endif
+/* Return true when FD file descriptor exists.  */
+
+int
+is_valid_fd (int fd)
+{
+#if defined(_WIN32)
+  HANDLE h = (HANDLE) _get_osfhandle (fd);
+  return h != (HANDLE) -1;
+#elif defined(F_GETFD)
+  return fcntl (fd, F_GETFD) >= 0;
+#else
+  return dup2 (fd, fd) < 0;
+#endif
+}
diff --git a/libiberty/lrealpath.c b/libiberty/lrealpath.c
index ac914a7a4f4..7f66dc2b1bd 100644
--- a/libiberty/lrealpath.c
+++ b/libiberty/lrealpath.c
@@ -49,9 +49,6 @@  components will be simplified.  The returned value will be allocated using
 #ifdef HAVE_STRING_H
 #include <string.h>
 #endif
-#ifdef HAVE_FCNTL_H
-#include <fcntl.h>
-#endif
 
 /* On GNU libc systems the declaration is only visible with _GNU_SOURCE.  */
 #if defined(HAVE_CANONICALIZE_FILE_NAME) \
@@ -158,16 +155,3 @@  lrealpath (const char *filename)
   /* This system is a lost cause, just duplicate the filename.  */
   return strdup (filename);
 }
-
-/* Return true when FD file descriptor exists.  */
-
-int
-is_valid_fd (int fd)
-{
-#if defined(_WIN32)
-  HANDLE h = (HANDLE) _get_osfhandle (fd);
-  return h != (HANDLE) -1;
-#else
-  return fcntl (fd, F_GETFD) >= 0;
-#endif
-}