| Message ID | c6b3a5e519cd3b0d7d77c7f2dfe35384b36cb39a.1581703185.git.fweimer@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | <fd_to_filename.h> improvements | expand |
Florian Weimer, le ven. 14 févr. 2020 19:11:09 +0100, a ecrit: > The new type struct fd_to_filename makes the allocation of the > backing storage explicit. > > Hurd uses /dev/fd, not /proc/self/fd. Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Thanks! > --- > libio/freopen.c | 4 +- > libio/freopen64.c | 4 +- > misc/Makefile | 6 +- > misc/fd_to_filename.c | 104 +++++++++++++++++ > misc/tst-fd_to_filename.c | 105 ++++++++++++++++++ > sysdeps/generic/arch-fd_to_filename.h | 19 ++++ > sysdeps/generic/fd_to_filename.h | 25 +++-- > sysdeps/mach/hurd/arch-fd_to_filename.h | 19 ++++ > ...fd_to_filename.h => arch-fd_to_filename.h} | 22 +--- > 9 files changed, 275 insertions(+), 33 deletions(-) > create mode 100644 misc/fd_to_filename.c > create mode 100644 misc/tst-fd_to_filename.c > create mode 100644 sysdeps/generic/arch-fd_to_filename.h > create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h > rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%) > > diff --git a/libio/freopen.c b/libio/freopen.c > index bab3ba204a..884cdb2961 100644 > --- a/libio/freopen.c > +++ b/libio/freopen.c > @@ -37,7 +37,7 @@ FILE * > freopen (const char *filename, const char *mode, FILE *fp) > { > FILE *result = NULL; > - char fdfilename[FD_TO_FILENAME_SIZE]; > + struct fd_to_filename fdfilename; > > CHECK_FILE (fp, NULL); > > @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp) > > int fd = _IO_fileno (fp); > const char *gfilename > - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); > + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); > > fp->_flags2 |= _IO_FLAGS2_NOCLOSE; > #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > diff --git a/libio/freopen64.c b/libio/freopen64.c > index c0ce604e6e..0d2c5264c7 100644 > --- a/libio/freopen64.c > +++ b/libio/freopen64.c > @@ -36,7 +36,7 @@ FILE * > freopen64 (const char *filename, const char *mode, FILE *fp) > { > FILE *result = NULL; > - char fdfilename[FD_TO_FILENAME_SIZE]; > + struct fd_to_filename fdfilename; > > CHECK_FILE (fp, NULL); > > @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) > > int fd = _IO_fileno (fp); > const char *gfilename > - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); > + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); > > fp->_flags2 |= _IO_FLAGS2_NOCLOSE; > _IO_file_close_it (fp); > diff --git a/misc/Makefile b/misc/Makefile > index e0465980c7..b8fed5783d 100644 > --- a/misc/Makefile > +++ b/misc/Makefile > @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \ > fgetxattr flistxattr fremovexattr fsetxattr getxattr \ > listxattr lgetxattr llistxattr lremovexattr lsetxattr \ > removexattr setxattr getauxval ifunc-impl-list makedev \ > - allocate_once > + allocate_once fd_to_filename > > generated += tst-error1.mtrace tst-error1-mem.out \ > tst-allocate_once.mtrace tst-allocate_once-mem.out > @@ -97,6 +97,10 @@ endif > tests-internal := tst-atomic tst-atomic-long tst-allocate_once > tests-static := tst-empty > > +# Test for the internal, non-exported __fd_to_filename function. > +tests-internal += tst-fd_to_filename > +tests-static += tst-fd_to_filename > + > ifeq ($(run-built-tests),yes) > tests-special += $(objpfx)tst-error1-mem.out \ > $(objpfx)tst-allocate_once-mem.out > diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c > new file mode 100644 > index 0000000000..911c22cf2b > --- /dev/null > +++ b/misc/fd_to_filename.c > @@ -0,0 +1,104 @@ > +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd). > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <fd_to_filename.h> > +#include <string.h> > + > +/* The split into groups of three digits is arbitrary, but it covers > + the common case of descriptors less than 1000 fairly > + efficiently. */ > + > +/* Writes two digits to P. VALUE must be between 0 and 99 > + (inclusive). Returns the address after the written digits. */ > +static char * > +digit2 (char *p, int value) > +{ > + p[0] = '0' + (value / 10); > + p[1] = '0' + (value % 10); > + return p + 2; > +} > + > +/* Writes three digits to P. VALUE must be between 0 and 999 > + (inclusive). Returns the address after the written digits. */ > +static char * > +digit3 (char *p, int value) > +{ > + p[0] = '0' + (value / 100); > + p[1] = '0' + ((value / 10) % 10); > + p[2] = '0' + (value % 10); > + return p + 3; > +} > + > +/* Writes one to three digits to P, depending on VALUE, which must be > + less than 1000. Returns the address after the written digits. */ > +static char * > +digit123 (char *p, int value) > +{ > + if (value < 10) > + { > + p[0] = '0' + value; > + return p + 1; > + } > + else if (value < 100) > + return digit2 (p, value); > + else > + return digit3 (p, value); > +} > + > +char * > +__fd_to_filename (int descriptor, struct fd_to_filename *storage) > +{ > + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, > + strlen (FD_TO_FILENAME_PREFIX)); > + if (__glibc_likely (descriptor >= 0)) > + { > + if (descriptor < 1000) > + p = digit123 (p, descriptor); > + else if (descriptor < 1000 * 1000) > + { > + p = digit123 (p, descriptor / 1000); > + p = digit3 (p, descriptor % 1000); > + } > + else if (descriptor < 1000 * 1000 * 1000) > + { > + p = digit123 (p, descriptor / (1000 * 1000)); > + p = digit3 (p, (descriptor / 1000) % 1000); > + p = digit3 (p, descriptor % 1000); > + } > + else > + { > + if (descriptor < 2000 * 1000 * 1000) > + { > + *p = '1'; > + descriptor -= 1000 * 1000 * 1000; > + } > + else > + { > + *p = '2'; > + descriptor -= 2000 * 1000 * 1000; > + } > + ++p; > + p = digit3 (p, descriptor / (1000 * 1000)); > + p = digit3 (p, (descriptor / 1000) % 1000); > + p = digit3 (p, descriptor % 1000); > + } > + } > + > + *p = '\0'; > + return storage->buffer; > +} > diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c > new file mode 100644 > index 0000000000..480f613dcf > --- /dev/null > +++ b/misc/tst-fd_to_filename.c > @@ -0,0 +1,105 @@ > +/* Test for /proc/self/fd (or /dev/fd) pathname construction. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <fd_to_filename.h> > +#include <stdio.h> > +#include <support/check.h> > +#include <support/xunistd.h> > + > +/* Run a check on one value. */ > +static void > +check (int value) > +{ > + struct fd_to_filename storage; > + char *actual = __fd_to_filename (value, &storage); > + char expected[100]; > + snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value); > + > + if (value < 0) > + /* Negative inputs are invalid, but the output should still be > + deterministic. */ > + TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX); > + else > + TEST_COMPARE_STRING (actual, expected); > +} > + > +/* Check various ranges constructed around powers. */ > +static void > +check_ranges (int base) > +{ > + unsigned int power = 1; > + do > + { > + for (int factor = 1; factor < base; ++factor) > + for (int shift = -1000; shift <= 1000; ++shift) > + { > + check (factor * power + shift); > + check (-(factor * power + shift)); > + } > + } > + while (!__builtin_mul_overflow (power, base, &power)); > +} > + > +/* Check that it is actually possible to use a the constructed > + name. */ > +static void > +check_open (void) > +{ > + int pipes[2]; > + xpipe (pipes); > + > + struct fd_to_filename storage; > + int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0); > + int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0); > + > + /* Ensure that all the descriptor numbers are different. */ > + TEST_VERIFY (pipes[0] < pipes[1]); > + TEST_VERIFY (pipes[1] < read_alias); > + TEST_VERIFY (read_alias < write_alias); > + > + xwrite (write_alias, "1", 1); > + char buf[16]; > + TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf))); > + > + xwrite (pipes[1], "2", 1); > + TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf))); > + > + xwrite (write_alias, "3", 1); > + TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf))); > + > + xwrite (pipes[1], "4", 1); > + TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf))); > + > + xclose (write_alias); > + xclose (read_alias); > + xclose (pipes[1]); > + xclose (pipes[0]); > +} > + > +static int > +do_test (void) > +{ > + check_ranges (2); > + check_ranges (10); > + > + check_open (); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h > new file mode 100644 > index 0000000000..ecaaa14dba > --- /dev/null > +++ b/sysdeps/generic/arch-fd_to_filename.h > @@ -0,0 +1,19 @@ > +/* Query filename corresponding to an open FD. Generic stub. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#error "<arch-fd_to_filename.h> must be ported to this architecture" > diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h > index eff6ca211b..7889db879c 100644 > --- a/sysdeps/generic/fd_to_filename.h > +++ b/sysdeps/generic/fd_to_filename.h > @@ -1,4 +1,4 @@ > -/* Query filename corresponding to an open FD. Generic version. > +/* Query filename corresponding to an open FD. > Copyright (C) 2001-2020 Free Software Foundation, Inc. > This file is part of the GNU C Library. > > @@ -16,12 +16,21 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#define FD_TO_FILENAME_SIZE 0 > +#ifndef _FD_TO_FILENAME_H > +#define _FD_TO_FILENAME_H > > -/* In general there is no generic way to query filename for an open > - file descriptor. */ > -static inline const char * > -fd_to_filename (int fd, char *buf) > +#include <arch-fd_to_filename.h> > + > +struct fd_to_filename > { > - return NULL; > -} > + /* A positive int value has at most 10 decimal digits. */ > + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10]; > +}; > + > +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and > + returns a pointer to the start of the string. DESCRIPTOR must be > + non-negative. */ > +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage) > + attribute_hidden; > + > +#endif /* _FD_TO_FILENAME_H */ > diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h > new file mode 100644 > index 0000000000..b45cd8d836 > --- /dev/null > +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h > @@ -0,0 +1,19 @@ > +/* Query filename corresponding to an open FD. Hurd version. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#define FD_TO_FILENAME_PREFIX "/dev/fd/" > diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h > similarity index 58% > rename from sysdeps/unix/sysv/linux/fd_to_filename.h > rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h > index 92a5e02976..b6017214c7 100644 > --- a/sysdeps/unix/sysv/linux/fd_to_filename.h > +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h > @@ -1,5 +1,5 @@ > /* Query filename corresponding to an open FD. Linux version. > - Copyright (C) 2001-2020 Free Software Foundation, Inc. > + Copyright (C) 2020 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 > @@ -16,22 +16,4 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include <sys/stat.h> > -#include <string.h> > -#include <_itoa.h> > - > -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \ > - + (sizeof ("4294967295") - 1) + 1) > - > -static inline const char * > -fd_to_filename (unsigned int fd, char *buf) > -{ > - *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0'; > - > - /* We must make sure the file exists. */ > - struct stat64 st; > - if (__lxstat64 (_STAT_VER, buf, &st) < 0) > - /* /proc is not mounted or something else happened. */ > - return NULL; > - return buf; > -} > +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/" > -- > 2.24.1 > >
On 14/02/2020 15:11, Florian Weimer wrote: > The new type struct fd_to_filename makes the allocation of the > backing storage explicit. > > Hurd uses /dev/fd, not /proc/self/fd. > --- > libio/freopen.c | 4 +- > libio/freopen64.c | 4 +- > misc/Makefile | 6 +- > misc/fd_to_filename.c | 104 +++++++++++++++++ > misc/tst-fd_to_filename.c | 105 ++++++++++++++++++ > sysdeps/generic/arch-fd_to_filename.h | 19 ++++ > sysdeps/generic/fd_to_filename.h | 25 +++-- > sysdeps/mach/hurd/arch-fd_to_filename.h | 19 ++++ > ...fd_to_filename.h => arch-fd_to_filename.h} | 22 +--- > 9 files changed, 275 insertions(+), 33 deletions(-) > create mode 100644 misc/fd_to_filename.c > create mode 100644 misc/tst-fd_to_filename.c > create mode 100644 sysdeps/generic/arch-fd_to_filename.h > create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h > rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%) > > diff --git a/libio/freopen.c b/libio/freopen.c > index bab3ba204a..884cdb2961 100644 > --- a/libio/freopen.c > +++ b/libio/freopen.c > @@ -37,7 +37,7 @@ FILE * > freopen (const char *filename, const char *mode, FILE *fp) > { > FILE *result = NULL; > - char fdfilename[FD_TO_FILENAME_SIZE]; > + struct fd_to_filename fdfilename; > > CHECK_FILE (fp, NULL); > > @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp) > > int fd = _IO_fileno (fp); > const char *gfilename > - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); > + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); > > fp->_flags2 |= _IO_FLAGS2_NOCLOSE; > #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) > diff --git a/libio/freopen64.c b/libio/freopen64.c > index c0ce604e6e..0d2c5264c7 100644 > --- a/libio/freopen64.c > +++ b/libio/freopen64.c > @@ -36,7 +36,7 @@ FILE * > freopen64 (const char *filename, const char *mode, FILE *fp) > { > FILE *result = NULL; > - char fdfilename[FD_TO_FILENAME_SIZE]; > + struct fd_to_filename fdfilename; > > CHECK_FILE (fp, NULL); > > @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) > > int fd = _IO_fileno (fp); > const char *gfilename > - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); > + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); > > fp->_flags2 |= _IO_FLAGS2_NOCLOSE; > _IO_file_close_it (fp); > diff --git a/misc/Makefile b/misc/Makefile > index e0465980c7..b8fed5783d 100644 > --- a/misc/Makefile > +++ b/misc/Makefile > @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \ > fgetxattr flistxattr fremovexattr fsetxattr getxattr \ > listxattr lgetxattr llistxattr lremovexattr lsetxattr \ > removexattr setxattr getauxval ifunc-impl-list makedev \ > - allocate_once > + allocate_once fd_to_filename > > generated += tst-error1.mtrace tst-error1-mem.out \ > tst-allocate_once.mtrace tst-allocate_once-mem.out > @@ -97,6 +97,10 @@ endif > tests-internal := tst-atomic tst-atomic-long tst-allocate_once > tests-static := tst-empty > > +# Test for the internal, non-exported __fd_to_filename function. > +tests-internal += tst-fd_to_filename > +tests-static += tst-fd_to_filename > + > ifeq ($(run-built-tests),yes) > tests-special += $(objpfx)tst-error1-mem.out \ > $(objpfx)tst-allocate_once-mem.out > diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c > new file mode 100644 > index 0000000000..911c22cf2b > --- /dev/null > +++ b/misc/fd_to_filename.c > @@ -0,0 +1,104 @@ > +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd). > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <fd_to_filename.h> > +#include <string.h> > + > +/* The split into groups of three digits is arbitrary, but it covers > + the common case of descriptors less than 1000 fairly > + efficiently. */ > + > +/* Writes two digits to P. VALUE must be between 0 and 99 > + (inclusive). Returns the address after the written digits. */ > +static char * > +digit2 (char *p, int value) > +{ > + p[0] = '0' + (value / 10); > + p[1] = '0' + (value % 10); > + return p + 2; > +} > + > +/* Writes three digits to P. VALUE must be between 0 and 999 > + (inclusive). Returns the address after the written digits. */ > +static char * > +digit3 (char *p, int value) > +{ > + p[0] = '0' + (value / 100); > + p[1] = '0' + ((value / 10) % 10); > + p[2] = '0' + (value % 10); > + return p + 3; > +} > + > +/* Writes one to three digits to P, depending on VALUE, which must be > + less than 1000. Returns the address after the written digits. */ > +static char * > +digit123 (char *p, int value) > +{ > + if (value < 10) > + { > + p[0] = '0' + value; > + return p + 1; > + } > + else if (value < 100) > + return digit2 (p, value); > + else > + return digit3 (p, value); > +} > + > +char * > +__fd_to_filename (int descriptor, struct fd_to_filename *storage) > +{ > + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, > + strlen (FD_TO_FILENAME_PREFIX)); > + if (__glibc_likely (descriptor >= 0)) > + { > + if (descriptor < 1000) > + p = digit123 (p, descriptor); > + else if (descriptor < 1000 * 1000) > + { > + p = digit123 (p, descriptor / 1000); > + p = digit3 (p, descriptor % 1000); > + } > + else if (descriptor < 1000 * 1000 * 1000) > + { > + p = digit123 (p, descriptor / (1000 * 1000)); > + p = digit3 (p, (descriptor / 1000) % 1000); > + p = digit3 (p, descriptor % 1000); > + } > + else > + { > + if (descriptor < 2000 * 1000 * 1000) > + { > + *p = '1'; > + descriptor -= 1000 * 1000 * 1000; > + } > + else > + { > + *p = '2'; > + descriptor -= 2000 * 1000 * 1000; > + } > + ++p; > + p = digit3 (p, descriptor / (1000 * 1000)); > + p = digit3 (p, (descriptor / 1000) % 1000); > + p = digit3 (p, descriptor % 1000); > + } > + } > + > + *p = '\0'; > + return storage->buffer; > +} Do we really such optimization which results in somewhat complicate code? Couldn't we use something simple as: char * __fd_to_filename (int descriptor, struct fd_to_filename *storage) { char *buf = storage->buffer; int i, j; for (i = 0; (buf[i] = FD_TO_FILENAME_PREFIX[i]); i++); if (descriptor == 0) { buf[i] = '0'; buf[i+1] = '\0'; return storage->buffer; } for (j = descriptor; j != 0; j /= 10, i++); buf[i] = '\0'; for (; descriptor != 0; descriptor /= 10) buf[--i] = '0' + descriptor % 10; return storage->buffer; } > diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c > new file mode 100644 > index 0000000000..480f613dcf > --- /dev/null > +++ b/misc/tst-fd_to_filename.c > @@ -0,0 +1,105 @@ > +/* Test for /proc/self/fd (or /dev/fd) pathname construction. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#include <fd_to_filename.h> > +#include <stdio.h> > +#include <support/check.h> > +#include <support/xunistd.h> > + > +/* Run a check on one value. */ > +static void > +check (int value) > +{ > + struct fd_to_filename storage; > + char *actual = __fd_to_filename (value, &storage); > + char expected[100]; > + snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value); > + > + if (value < 0) > + /* Negative inputs are invalid, but the output should still be > + deterministic. */ > + TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX); > + else > + TEST_COMPARE_STRING (actual, expected); > +} > + > +/* Check various ranges constructed around powers. */ > +static void > +check_ranges (int base) > +{ > + unsigned int power = 1; > + do > + { > + for (int factor = 1; factor < base; ++factor) > + for (int shift = -1000; shift <= 1000; ++shift) > + { > + check (factor * power + shift); > + check (-(factor * power + shift)); > + } > + } > + while (!__builtin_mul_overflow (power, base, &power)); > +} > + > +/* Check that it is actually possible to use a the constructed > + name. */ > +static void > +check_open (void) > +{ > + int pipes[2]; > + xpipe (pipes); > + > + struct fd_to_filename storage; > + int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0); > + int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0); > + > + /* Ensure that all the descriptor numbers are different. */ > + TEST_VERIFY (pipes[0] < pipes[1]); > + TEST_VERIFY (pipes[1] < read_alias); > + TEST_VERIFY (read_alias < write_alias); > + > + xwrite (write_alias, "1", 1); > + char buf[16]; > + TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf))); > + > + xwrite (pipes[1], "2", 1); > + TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf))); > + > + xwrite (write_alias, "3", 1); > + TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf))); > + > + xwrite (pipes[1], "4", 1); > + TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf))); > + > + xclose (write_alias); > + xclose (read_alias); > + xclose (pipes[1]); > + xclose (pipes[0]); > +} > + > +static int > +do_test (void) > +{ > + check_ranges (2); > + check_ranges (10); > + > + check_open (); > + > + return 0; > +} > + > +#include <support/test-driver.c> > diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h > new file mode 100644 > index 0000000000..ecaaa14dba > --- /dev/null > +++ b/sysdeps/generic/arch-fd_to_filename.h > @@ -0,0 +1,19 @@ > +/* Query filename corresponding to an open FD. Generic stub. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#error "<arch-fd_to_filename.h> must be ported to this architecture" > diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h > index eff6ca211b..7889db879c 100644 > --- a/sysdeps/generic/fd_to_filename.h > +++ b/sysdeps/generic/fd_to_filename.h > @@ -1,4 +1,4 @@ > -/* Query filename corresponding to an open FD. Generic version. > +/* Query filename corresponding to an open FD. > Copyright (C) 2001-2020 Free Software Foundation, Inc. > This file is part of the GNU C Library. > > @@ -16,12 +16,21 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#define FD_TO_FILENAME_SIZE 0 > +#ifndef _FD_TO_FILENAME_H > +#define _FD_TO_FILENAME_H > > -/* In general there is no generic way to query filename for an open > - file descriptor. */ > -static inline const char * > -fd_to_filename (int fd, char *buf) > +#include <arch-fd_to_filename.h> > + > +struct fd_to_filename > { > - return NULL; > -} > + /* A positive int value has at most 10 decimal digits. */ > + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10]; > +}; > + > +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and > + returns a pointer to the start of the string. DESCRIPTOR must be > + non-negative. */ > +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage) > + attribute_hidden; > + > +#endif /* _FD_TO_FILENAME_H */ > diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h > new file mode 100644 > index 0000000000..b45cd8d836 > --- /dev/null > +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h > @@ -0,0 +1,19 @@ > +/* Query filename corresponding to an open FD. Hurd version. > + Copyright (C) 2020 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 > + <https://www.gnu.org/licenses/>. */ > + > +#define FD_TO_FILENAME_PREFIX "/dev/fd/" > diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h > similarity index 58% > rename from sysdeps/unix/sysv/linux/fd_to_filename.h > rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h > index 92a5e02976..b6017214c7 100644 > --- a/sysdeps/unix/sysv/linux/fd_to_filename.h > +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h > @@ -1,5 +1,5 @@ > /* Query filename corresponding to an open FD. Linux version. > - Copyright (C) 2001-2020 Free Software Foundation, Inc. > + Copyright (C) 2020 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 > @@ -16,22 +16,4 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > -#include <sys/stat.h> > -#include <string.h> > -#include <_itoa.h> > - > -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \ > - + (sizeof ("4294967295") - 1) + 1) > - > -static inline const char * > -fd_to_filename (unsigned int fd, char *buf) > -{ > - *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0'; > - > - /* We must make sure the file exists. */ > - struct stat64 st; > - if (__lxstat64 (_STAT_VER, buf, &st) < 0) > - /* /proc is not mounted or something else happened. */ > - return NULL; > - return buf; > -} > +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/" >
* Adhemerval Zanella: > Do we really such optimization which results in somewhat complicate > code? Couldn't we use something simple as: > > char * > __fd_to_filename (int descriptor, struct fd_to_filename *storage) > { > char *buf = storage->buffer; > int i, j; > > for (i = 0; (buf[i] = FD_TO_FILENAME_PREFIX[i]); i++); > if (descriptor == 0) > { > buf[i] = '0'; > buf[i+1] = '\0'; > return storage->buffer; > } > > for (j = descriptor; j != 0; j /= 10, i++); > buf[i] = '\0'; > for (; descriptor != 0; descriptor /= 10) > buf[--i] = '0' + descriptor % 10; > > return storage->buffer; > } I don't have a strong opinion about this (but I think we should probably keep the mempcpy). Most descriptors will be fairly small, so the smaller code size is probably more important. Thanks, Florian
On 2/14/20 10:11 AM, Florian Weimer wrote: > + /* A positive int value has at most 10 decimal digits. */ > + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10]; This should use 'INT_STRLEN_BOUND (int)' rather than '10', where INT_STRLEN_BOUND is taken from intprops.h. Then you don't need the comment (and the code won't break on future ILP64 platforms :-). > +char * > +__fd_to_filename (int descriptor, struct fd_to_filename *storage) > +{ > + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, > + strlen (FD_TO_FILENAME_PREFIX)); > + if (__glibc_likely (descriptor >= 0)) > + { > + if (descriptor < 1000) > + p = digit123 (p, descriptor); > + else if (descriptor < 1000 * 1000) ... It's not clear what that "descriptor >= 0" test is doing. I assume that a precondition is that DESCRIPTOR is nonnegative; if so, this should be mentioned and the test removed. If it's not a precondition, the code should do the right thing if DESCRIPTOR is negative (I suppose, return a filename that cannot be opened, though it doesn't do that currently) -- which should also be documented but I think this'd be overkill. As Adhemerval writes, there's no need for special cases for powers of 1000 etc. Something like the following should do (this is a bit shorter than Adhemerval's version): /* Convert nonnegative DESCRIPTOR to a file name. */ char * __fd_to_filename (int descriptor, struct fd_to_filename *storage) { char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, strlen (FD_TO_FILENAME_PREFIX) - 1); for (int d = descriptor; p++, (d /= 10) != 0; ) continue; *p = '\0'; for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; ) continue; return storage->buffer; } GCC turns those divisions and remainders into the usual multiply+shift+add and this should be good enough.
* Paul Eggert: > On 2/14/20 10:11 AM, Florian Weimer wrote: >> + /* A positive int value has at most 10 decimal digits. */ >> + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10]; > > This should use 'INT_STRLEN_BOUND (int)' rather than '10', where > INT_STRLEN_BOUND is taken from intprops.h. Then you don't need the > comment (and the code won't break on future ILP64 platforms :-). But INT_STRLEN_BOUND is 11, right? >> +char * >> +__fd_to_filename (int descriptor, struct fd_to_filename *storage) >> +{ >> + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, >> + strlen (FD_TO_FILENAME_PREFIX)); >> + if (__glibc_likely (descriptor >= 0)) >> + { >> + if (descriptor < 1000) >> + p = digit123 (p, descriptor); >> + else if (descriptor < 1000 * 1000) ... > > It's not clear what that "descriptor >= 0" test is doing. I assume > that a precondition is that DESCRIPTOR is nonnegative; if so, this > should be mentioned and the test removed. If it's not a precondition, > the code should do the right thing if DESCRIPTOR is negative (I > suppose, return a filename that cannot be opened, though it doesn't do > that currently) -- which should also be documented but I think this'd > be overkill. The problem is when an application passes an invalid descriptor to some libc function and that ends up with __fd_to_filename. We should not make matters worse in that case. Maybe we should use an assert? Thanks, Florian
On 2/15/20 5:16 AM, Florian Weimer wrote: > INT_STRLEN_BOUND is 11, right? Yes, it's a bound on the string length of a printed int, and that's 11 in the typical case of 32-bit int because the int might be negative. I didn't lose sleep over the wasted byte, but if we want a tighter bound then we could use INT_STRLEN_BOUND (int) - 1 instead. However, it might be better to leave it alone so that we can use the code below. > The problem is when an application passes an invalid descriptor to some > libc function and that ends up with __fd_to_filename. We should not > make matters worse in that case. If it's not a precondition that the descriptor is nonnegative, we can't simply return a copy of FD_TO_FILENAME_PREFIX as that's an existing filename. Instead, how about the following? It uses a randomish garbage filename beginning with "-" which should be good enough, and it doesn't cost a conditional branch to handle negative descriptors. char * __fd_to_filename (int descriptor, struct fd_to_filename *storage) { char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, strlen (FD_TO_FILENAME_PREFIX) - 1); /* If DESCRIPTOR is negative, arrange for the filename to not exist by prepending any byte other than '/', '.', '\0' or an ASCII digit. The rest of the filename will be gibberish that fits. */ *p = '-'; p += descriptor < 0; for (int d = descriptor; p++, (d /= 10) != 0; ) continue; *p = '\0'; for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; ) continue; return storage->buffer; }
* Paul Eggert: > On 2/15/20 5:16 AM, Florian Weimer wrote: > >> INT_STRLEN_BOUND is 11, right? > > Yes, it's a bound on the string length of a printed int, and that's 11 > in the typical case of 32-bit int because the int might be negative. > I didn't lose sleep over the wasted byte, but if we want a tighter > bound then we could use INT_STRLEN_BOUND (int) - 1 instead. However, > it might be better to leave it alone so that we can use the code > below. > >> The problem is when an application passes an invalid descriptor to some >> libc function and that ends up with __fd_to_filename. We should not >> make matters worse in that case. > > If it's not a precondition that the descriptor is nonnegative, we > can't simply return a copy of FD_TO_FILENAME_PREFIX as that's an > existing filename. Instead, how about the following? It uses a > randomish garbage filename beginning with "-" > which should be good enough, and it doesn't cost a conditional branch > to handle negative descriptors. > > char * > __fd_to_filename (int descriptor, struct fd_to_filename *storage) > { > char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, > strlen (FD_TO_FILENAME_PREFIX) - 1); > > /* If DESCRIPTOR is negative, arrange for the filename to not exist > by prepending any byte other than '/', '.', '\0' or an ASCII digit. > The rest of the filename will be gibberish that fits. */ > *p = '-'; > p += descriptor < 0; > > for (int d = descriptor; p++, (d /= 10) != 0; ) > continue; > *p = '\0'; > for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; ) > continue; > return storage->buffer; > } Here's an updated version, which adds a dependency on <intprops.h> (a header I really dislike) and mostly uses your implementation of __fd_to_filename. Okay for master? Thanks, Florian 8<------------------------------------------------------------------8< The new type struct fd_to_filename makes the allocation of the backing storage explicit. Hurd uses /dev/fd, not /proc/self/fd. Co-Authored-By: Paul Eggert <eggert@cs.ucla.edu> ----- libio/freopen.c | 4 +- libio/freopen64.c | 4 +- misc/Makefile | 6 +- misc/fd_to_filename.c | 38 ++++++++ misc/tst-fd_to_filename.c | 100 +++++++++++++++++++++ sysdeps/generic/arch-fd_to_filename.h | 19 ++++ sysdeps/generic/fd_to_filename.h | 26 ++++-- sysdeps/mach/hurd/arch-fd_to_filename.h | 19 ++++ .../{fd_to_filename.h => arch-fd_to_filename.h} | 22 +---- 9 files changed, 205 insertions(+), 33 deletions(-) diff --git a/libio/freopen.c b/libio/freopen.c index bab3ba204a..884cdb2961 100644 --- a/libio/freopen.c +++ b/libio/freopen.c @@ -37,7 +37,7 @@ FILE * freopen (const char *filename, const char *mode, FILE *fp) { FILE *result = NULL; - char fdfilename[FD_TO_FILENAME_SIZE]; + struct fd_to_filename fdfilename; CHECK_FILE (fp, NULL); @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp) int fd = _IO_fileno (fp); const char *gfilename - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); fp->_flags2 |= _IO_FLAGS2_NOCLOSE; #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) diff --git a/libio/freopen64.c b/libio/freopen64.c index c0ce604e6e..0d2c5264c7 100644 --- a/libio/freopen64.c +++ b/libio/freopen64.c @@ -36,7 +36,7 @@ FILE * freopen64 (const char *filename, const char *mode, FILE *fp) { FILE *result = NULL; - char fdfilename[FD_TO_FILENAME_SIZE]; + struct fd_to_filename fdfilename; CHECK_FILE (fp, NULL); @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) int fd = _IO_fileno (fp); const char *gfilename - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); fp->_flags2 |= _IO_FLAGS2_NOCLOSE; _IO_file_close_it (fp); diff --git a/misc/Makefile b/misc/Makefile index e0465980c7..b8fed5783d 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \ fgetxattr flistxattr fremovexattr fsetxattr getxattr \ listxattr lgetxattr llistxattr lremovexattr lsetxattr \ removexattr setxattr getauxval ifunc-impl-list makedev \ - allocate_once + allocate_once fd_to_filename generated += tst-error1.mtrace tst-error1-mem.out \ tst-allocate_once.mtrace tst-allocate_once-mem.out @@ -97,6 +97,10 @@ endif tests-internal := tst-atomic tst-atomic-long tst-allocate_once tests-static := tst-empty +# Test for the internal, non-exported __fd_to_filename function. +tests-internal += tst-fd_to_filename +tests-static += tst-fd_to_filename + ifeq ($(run-built-tests),yes) tests-special += $(objpfx)tst-error1-mem.out \ $(objpfx)tst-allocate_once-mem.out diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c new file mode 100644 index 0000000000..03d19194c1 --- /dev/null +++ b/misc/fd_to_filename.c @@ -0,0 +1,38 @@ +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd). + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#include <fd_to_filename.h> + +#include <assert.h> +#include <string.h> + +char * +__fd_to_filename (int descriptor, struct fd_to_filename *storage) +{ + assert (descriptor >= 0); + + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, + strlen (FD_TO_FILENAME_PREFIX)); + + for (int d = descriptor; p++, (d /= 10) != 0; ) + continue; + *p = '\0'; + for (int d = descriptor; *--p = '0' + d % 10, (d /= 10) != 0; ) + continue; + return storage->buffer; +} diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c new file mode 100644 index 0000000000..3a3bccdbcf --- /dev/null +++ b/misc/tst-fd_to_filename.c @@ -0,0 +1,100 @@ +/* Test for /proc/self/fd (or /dev/fd) pathname construction. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#include <fd_to_filename.h> +#include <stdio.h> +#include <support/check.h> +#include <support/xunistd.h> + +/* Run a check on one value. */ +static void +check (int value) +{ + if (value < 0) + /* Negative descriptor values violate the precondition. */ + return; + + struct fd_to_filename storage; + char *actual = __fd_to_filename (value, &storage); + char expected[100]; + snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value); + TEST_COMPARE_STRING (actual, expected); +} + +/* Check various ranges constructed around powers. */ +static void +check_ranges (int base) +{ + unsigned int power = 1; + do + { + for (int factor = 1; factor < base; ++factor) + for (int shift = -1000; shift <= 1000; ++shift) + check (factor * power + shift); + } + while (!__builtin_mul_overflow (power, base, &power)); +} + +/* Check that it is actually possible to use a the constructed + name. */ +static void +check_open (void) +{ + int pipes[2]; + xpipe (pipes); + + struct fd_to_filename storage; + int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0); + int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0); + + /* Ensure that all the descriptor numbers are different. */ + TEST_VERIFY (pipes[0] < pipes[1]); + TEST_VERIFY (pipes[1] < read_alias); + TEST_VERIFY (read_alias < write_alias); + + xwrite (write_alias, "1", 1); + char buf[16]; + TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf))); + + xwrite (pipes[1], "2", 1); + TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf))); + + xwrite (write_alias, "3", 1); + TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf))); + + xwrite (pipes[1], "4", 1); + TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf))); + + xclose (write_alias); + xclose (read_alias); + xclose (pipes[1]); + xclose (pipes[0]); +} + +static int +do_test (void) +{ + check_ranges (2); + check_ranges (10); + + check_open (); + + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h new file mode 100644 index 0000000000..ecaaa14dba --- /dev/null +++ b/sysdeps/generic/arch-fd_to_filename.h @@ -0,0 +1,19 @@ +/* Query filename corresponding to an open FD. Generic stub. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#error "<arch-fd_to_filename.h> must be ported to this architecture" diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h index eff6ca211b..5ca22f02bc 100644 --- a/sysdeps/generic/fd_to_filename.h +++ b/sysdeps/generic/fd_to_filename.h @@ -1,4 +1,4 @@ -/* Query filename corresponding to an open FD. Generic version. +/* Query filename corresponding to an open FD. Copyright (C) 2001-2020 Free Software Foundation, Inc. This file is part of the GNU C Library. @@ -16,12 +16,22 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#define FD_TO_FILENAME_SIZE 0 +#ifndef _FD_TO_FILENAME_H +#define _FD_TO_FILENAME_H -/* In general there is no generic way to query filename for an open - file descriptor. */ -static inline const char * -fd_to_filename (int fd, char *buf) +#include <arch-fd_to_filename.h> +#include <intprops.h> + +struct fd_to_filename { - return NULL; -} + /* A positive int value has at most 10 decimal digits. */ + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + INT_STRLEN_BOUND (int)]; +}; + +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and + returns a pointer to the start of the string. DESCRIPTOR must be + non-negative. */ +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage) + attribute_hidden; + +#endif /* _FD_TO_FILENAME_H */ diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h new file mode 100644 index 0000000000..b45cd8d836 --- /dev/null +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h @@ -0,0 +1,19 @@ +/* Query filename corresponding to an open FD. Hurd version. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#define FD_TO_FILENAME_PREFIX "/dev/fd/" diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h similarity index 58% rename from sysdeps/unix/sysv/linux/fd_to_filename.h rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h index 92a5e02976..b6017214c7 100644 --- a/sysdeps/unix/sysv/linux/fd_to_filename.h +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h @@ -1,5 +1,5 @@ /* Query filename corresponding to an open FD. Linux version. - Copyright (C) 2001-2020 Free Software Foundation, Inc. + Copyright (C) 2020 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 @@ -16,22 +16,4 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <sys/stat.h> -#include <string.h> -#include <_itoa.h> - -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \ - + (sizeof ("4294967295") - 1) + 1) - -static inline const char * -fd_to_filename (unsigned int fd, char *buf) -{ - *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0'; - - /* We must make sure the file exists. */ - struct stat64 st; - if (__lxstat64 (_STAT_VER, buf, &st) < 0) - /* /proc is not mounted or something else happened. */ - return NULL; - return buf; -} +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"
On 2/17/20 7:18 AM, Florian Weimer wrote: > Here's an updated version Thanks, it looks good. > which adds a dependency on <intprops.h> (a > header I really dislike) I disliked *writing* intprops.h (what a mess that part of C is!) but I don't dislike *using* it when it's clearer or more reliable than the alternatives.
On 17/02/2020 15:26, Paul Eggert wrote: > On 2/17/20 7:18 AM, Florian Weimer wrote: >> Here's an updated version > > Thanks, it looks good. > >> which adds a dependency on <intprops.h> (a >> header I really dislike) > > I disliked *writing* intprops.h (what a mess that part of C is!) but I don't dislike *using* it when it's clearer or more reliable than the alternatives. I think the main problem with intprops.h is it contains a lot of clutter not really useful for the current glibc targets, and it could be simplified with current minimum build compiler requirement. Also, although its usage is indeed handy in some situations, its implementation is somewhat complex and convoluted. I don't see a better solution with given constraint though. So the question is if it still worth to keep it in sync with gnulib, or if is better to push for a more tailored and clean replacement.
On 2/17/20 10:59 AM, Adhemerval Zanella wrote: > So the question is if it still worth to keep it in sync with > gnulib, or if is better to push for a more tailored and clean > replacement. Another option would be to clarify which part is used in glibc, and which part is needed only for Gnulib. We do that in some other files shared between the two projects, by using "#if _LIBC" and the like.
diff --git a/libio/freopen.c b/libio/freopen.c index bab3ba204a..884cdb2961 100644 --- a/libio/freopen.c +++ b/libio/freopen.c @@ -37,7 +37,7 @@ FILE * freopen (const char *filename, const char *mode, FILE *fp) { FILE *result = NULL; - char fdfilename[FD_TO_FILENAME_SIZE]; + struct fd_to_filename fdfilename; CHECK_FILE (fp, NULL); @@ -50,7 +50,7 @@ freopen (const char *filename, const char *mode, FILE *fp) int fd = _IO_fileno (fp); const char *gfilename - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); fp->_flags2 |= _IO_FLAGS2_NOCLOSE; #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_1) diff --git a/libio/freopen64.c b/libio/freopen64.c index c0ce604e6e..0d2c5264c7 100644 --- a/libio/freopen64.c +++ b/libio/freopen64.c @@ -36,7 +36,7 @@ FILE * freopen64 (const char *filename, const char *mode, FILE *fp) { FILE *result = NULL; - char fdfilename[FD_TO_FILENAME_SIZE]; + struct fd_to_filename fdfilename; CHECK_FILE (fp, NULL); @@ -49,7 +49,7 @@ freopen64 (const char *filename, const char *mode, FILE *fp) int fd = _IO_fileno (fp); const char *gfilename - = filename != NULL ? filename : fd_to_filename (fd, fdfilename); + = filename != NULL ? filename : __fd_to_filename (fd, &fdfilename); fp->_flags2 |= _IO_FLAGS2_NOCLOSE; _IO_file_close_it (fp); diff --git a/misc/Makefile b/misc/Makefile index e0465980c7..b8fed5783d 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -72,7 +72,7 @@ routines := brk sbrk sstk ioctl \ fgetxattr flistxattr fremovexattr fsetxattr getxattr \ listxattr lgetxattr llistxattr lremovexattr lsetxattr \ removexattr setxattr getauxval ifunc-impl-list makedev \ - allocate_once + allocate_once fd_to_filename generated += tst-error1.mtrace tst-error1-mem.out \ tst-allocate_once.mtrace tst-allocate_once-mem.out @@ -97,6 +97,10 @@ endif tests-internal := tst-atomic tst-atomic-long tst-allocate_once tests-static := tst-empty +# Test for the internal, non-exported __fd_to_filename function. +tests-internal += tst-fd_to_filename +tests-static += tst-fd_to_filename + ifeq ($(run-built-tests),yes) tests-special += $(objpfx)tst-error1-mem.out \ $(objpfx)tst-allocate_once-mem.out diff --git a/misc/fd_to_filename.c b/misc/fd_to_filename.c new file mode 100644 index 0000000000..911c22cf2b --- /dev/null +++ b/misc/fd_to_filename.c @@ -0,0 +1,104 @@ +/* Construct a pathname under /proc/self/fd (or /dev/fd for Hurd). + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#include <fd_to_filename.h> +#include <string.h> + +/* The split into groups of three digits is arbitrary, but it covers + the common case of descriptors less than 1000 fairly + efficiently. */ + +/* Writes two digits to P. VALUE must be between 0 and 99 + (inclusive). Returns the address after the written digits. */ +static char * +digit2 (char *p, int value) +{ + p[0] = '0' + (value / 10); + p[1] = '0' + (value % 10); + return p + 2; +} + +/* Writes three digits to P. VALUE must be between 0 and 999 + (inclusive). Returns the address after the written digits. */ +static char * +digit3 (char *p, int value) +{ + p[0] = '0' + (value / 100); + p[1] = '0' + ((value / 10) % 10); + p[2] = '0' + (value % 10); + return p + 3; +} + +/* Writes one to three digits to P, depending on VALUE, which must be + less than 1000. Returns the address after the written digits. */ +static char * +digit123 (char *p, int value) +{ + if (value < 10) + { + p[0] = '0' + value; + return p + 1; + } + else if (value < 100) + return digit2 (p, value); + else + return digit3 (p, value); +} + +char * +__fd_to_filename (int descriptor, struct fd_to_filename *storage) +{ + char *p = mempcpy (storage->buffer, FD_TO_FILENAME_PREFIX, + strlen (FD_TO_FILENAME_PREFIX)); + if (__glibc_likely (descriptor >= 0)) + { + if (descriptor < 1000) + p = digit123 (p, descriptor); + else if (descriptor < 1000 * 1000) + { + p = digit123 (p, descriptor / 1000); + p = digit3 (p, descriptor % 1000); + } + else if (descriptor < 1000 * 1000 * 1000) + { + p = digit123 (p, descriptor / (1000 * 1000)); + p = digit3 (p, (descriptor / 1000) % 1000); + p = digit3 (p, descriptor % 1000); + } + else + { + if (descriptor < 2000 * 1000 * 1000) + { + *p = '1'; + descriptor -= 1000 * 1000 * 1000; + } + else + { + *p = '2'; + descriptor -= 2000 * 1000 * 1000; + } + ++p; + p = digit3 (p, descriptor / (1000 * 1000)); + p = digit3 (p, (descriptor / 1000) % 1000); + p = digit3 (p, descriptor % 1000); + } + } + + *p = '\0'; + return storage->buffer; +} diff --git a/misc/tst-fd_to_filename.c b/misc/tst-fd_to_filename.c new file mode 100644 index 0000000000..480f613dcf --- /dev/null +++ b/misc/tst-fd_to_filename.c @@ -0,0 +1,105 @@ +/* Test for /proc/self/fd (or /dev/fd) pathname construction. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#include <fd_to_filename.h> +#include <stdio.h> +#include <support/check.h> +#include <support/xunistd.h> + +/* Run a check on one value. */ +static void +check (int value) +{ + struct fd_to_filename storage; + char *actual = __fd_to_filename (value, &storage); + char expected[100]; + snprintf (expected, sizeof (expected), FD_TO_FILENAME_PREFIX "%d", value); + + if (value < 0) + /* Negative inputs are invalid, but the output should still be + deterministic. */ + TEST_COMPARE_STRING (actual, FD_TO_FILENAME_PREFIX); + else + TEST_COMPARE_STRING (actual, expected); +} + +/* Check various ranges constructed around powers. */ +static void +check_ranges (int base) +{ + unsigned int power = 1; + do + { + for (int factor = 1; factor < base; ++factor) + for (int shift = -1000; shift <= 1000; ++shift) + { + check (factor * power + shift); + check (-(factor * power + shift)); + } + } + while (!__builtin_mul_overflow (power, base, &power)); +} + +/* Check that it is actually possible to use a the constructed + name. */ +static void +check_open (void) +{ + int pipes[2]; + xpipe (pipes); + + struct fd_to_filename storage; + int read_alias = xopen (__fd_to_filename (pipes[0], &storage), O_RDONLY, 0); + int write_alias = xopen (__fd_to_filename (pipes[1], &storage), O_WRONLY, 0); + + /* Ensure that all the descriptor numbers are different. */ + TEST_VERIFY (pipes[0] < pipes[1]); + TEST_VERIFY (pipes[1] < read_alias); + TEST_VERIFY (read_alias < write_alias); + + xwrite (write_alias, "1", 1); + char buf[16]; + TEST_COMPARE_BLOB ("1", 1, buf, read (pipes[0], buf, sizeof (buf))); + + xwrite (pipes[1], "2", 1); + TEST_COMPARE_BLOB ("2", 1, buf, read (read_alias, buf, sizeof (buf))); + + xwrite (write_alias, "3", 1); + TEST_COMPARE_BLOB ("3", 1, buf, read (read_alias, buf, sizeof (buf))); + + xwrite (pipes[1], "4", 1); + TEST_COMPARE_BLOB ("4", 1, buf, read (pipes[0], buf, sizeof (buf))); + + xclose (write_alias); + xclose (read_alias); + xclose (pipes[1]); + xclose (pipes[0]); +} + +static int +do_test (void) +{ + check_ranges (2); + check_ranges (10); + + check_open (); + + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/generic/arch-fd_to_filename.h b/sysdeps/generic/arch-fd_to_filename.h new file mode 100644 index 0000000000..ecaaa14dba --- /dev/null +++ b/sysdeps/generic/arch-fd_to_filename.h @@ -0,0 +1,19 @@ +/* Query filename corresponding to an open FD. Generic stub. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#error "<arch-fd_to_filename.h> must be ported to this architecture" diff --git a/sysdeps/generic/fd_to_filename.h b/sysdeps/generic/fd_to_filename.h index eff6ca211b..7889db879c 100644 --- a/sysdeps/generic/fd_to_filename.h +++ b/sysdeps/generic/fd_to_filename.h @@ -1,4 +1,4 @@ -/* Query filename corresponding to an open FD. Generic version. +/* Query filename corresponding to an open FD. Copyright (C) 2001-2020 Free Software Foundation, Inc. This file is part of the GNU C Library. @@ -16,12 +16,21 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#define FD_TO_FILENAME_SIZE 0 +#ifndef _FD_TO_FILENAME_H +#define _FD_TO_FILENAME_H -/* In general there is no generic way to query filename for an open - file descriptor. */ -static inline const char * -fd_to_filename (int fd, char *buf) +#include <arch-fd_to_filename.h> + +struct fd_to_filename { - return NULL; -} + /* A positive int value has at most 10 decimal digits. */ + char buffer[sizeof (FD_TO_FILENAME_PREFIX) + 10]; +}; + +/* Writes a /proc/self/fd-style path for DESCRIPTOR to *STORAGE and + returns a pointer to the start of the string. DESCRIPTOR must be + non-negative. */ +char *__fd_to_filename (int descriptor, struct fd_to_filename *storage) + attribute_hidden; + +#endif /* _FD_TO_FILENAME_H */ diff --git a/sysdeps/mach/hurd/arch-fd_to_filename.h b/sysdeps/mach/hurd/arch-fd_to_filename.h new file mode 100644 index 0000000000..b45cd8d836 --- /dev/null +++ b/sysdeps/mach/hurd/arch-fd_to_filename.h @@ -0,0 +1,19 @@ +/* Query filename corresponding to an open FD. Hurd version. + Copyright (C) 2020 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 + <https://www.gnu.org/licenses/>. */ + +#define FD_TO_FILENAME_PREFIX "/dev/fd/" diff --git a/sysdeps/unix/sysv/linux/fd_to_filename.h b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h similarity index 58% rename from sysdeps/unix/sysv/linux/fd_to_filename.h rename to sysdeps/unix/sysv/linux/arch-fd_to_filename.h index 92a5e02976..b6017214c7 100644 --- a/sysdeps/unix/sysv/linux/fd_to_filename.h +++ b/sysdeps/unix/sysv/linux/arch-fd_to_filename.h @@ -1,5 +1,5 @@ /* Query filename corresponding to an open FD. Linux version. - Copyright (C) 2001-2020 Free Software Foundation, Inc. + Copyright (C) 2020 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 @@ -16,22 +16,4 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ -#include <sys/stat.h> -#include <string.h> -#include <_itoa.h> - -#define FD_TO_FILENAME_SIZE ((sizeof ("/proc/self/fd/") - 1) \ - + (sizeof ("4294967295") - 1) + 1) - -static inline const char * -fd_to_filename (unsigned int fd, char *buf) -{ - *_fitoa_word (fd, __stpcpy (buf, "/proc/self/fd/"), 10, 0) = '\0'; - - /* We must make sure the file exists. */ - struct stat64 st; - if (__lxstat64 (_STAT_VER, buf, &st) < 0) - /* /proc is not mounted or something else happened. */ - return NULL; - return buf; -} +#define FD_TO_FILENAME_PREFIX "/proc/self/fd/"
The new type struct fd_to_filename makes the allocation of the backing storage explicit. Hurd uses /dev/fd, not /proc/self/fd. --- libio/freopen.c | 4 +- libio/freopen64.c | 4 +- misc/Makefile | 6 +- misc/fd_to_filename.c | 104 +++++++++++++++++ misc/tst-fd_to_filename.c | 105 ++++++++++++++++++ sysdeps/generic/arch-fd_to_filename.h | 19 ++++ sysdeps/generic/fd_to_filename.h | 25 +++-- sysdeps/mach/hurd/arch-fd_to_filename.h | 19 ++++ ...fd_to_filename.h => arch-fd_to_filename.h} | 22 +--- 9 files changed, 275 insertions(+), 33 deletions(-) create mode 100644 misc/fd_to_filename.c create mode 100644 misc/tst-fd_to_filename.c create mode 100644 sysdeps/generic/arch-fd_to_filename.h create mode 100644 sysdeps/mach/hurd/arch-fd_to_filename.h rename sysdeps/unix/sysv/linux/{fd_to_filename.h => arch-fd_to_filename.h} (58%)