Message ID | 1476525379-2949-1-git-send-email-ynorov@caviumnetworks.com |
---|---|
State | New |
Headers | show |
On Okt 15 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: > diff --git a/sysdeps/unix/sysv/linux/rename.c b/sysdeps/unix/sysv/linux/rename.c > new file mode 100644 > index 0000000..62a58ae > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/rename.c > @@ -0,0 +1,34 @@ > +/* rename() syscall > + Copyright (C) 2016 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. > + > + 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 <stdio.h> > +#include <unistd.h> > +#include <fcntl.h> > +#include <sysdep.h> > + > +/* Rename the file OLD to NEW. */ > +int > +rename (const char *old, const char *new) > +{ > +#ifdef __NR_renameat2 > + return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); > +#else > + return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); > +#endif That breaks all kernels that don't implement renameat2. And it doesn't compile: ../sysdeps/unix/sysv/linux/rename.c: In function ‘rename’: ../sysdeps/unix/sysv/linux/rename.c:30:3: error: implicit declaration of function ‘__set_errno’ [-Werror=implicit-function-declaration] return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); ^ > diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list > index 7ae2541..a2c1060 100644 > --- a/sysdeps/unix/sysv/linux/syscalls.list > +++ b/sysdeps/unix/sysv/linux/syscalls.list > @@ -89,7 +89,6 @@ fchownat - fchownat i:isiii fchownat > linkat - linkat i:isisi linkat > mkdirat - mkdirat i:isi mkdirat > readlinkat - readlinkat i:issi readlinkat > -renameat - renameat i:isis renameat > symlinkat - symlinkat i:sis symlinkat > unlinkat - unlinkat i:isi unlinkat The only other implementation of renameat is the stub in stdio-common which always fails. Andreas.
On Sat, Oct 15, 2016 at 03:02:27PM +0200, Andreas Schwab wrote: > On Okt 15 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: > > > diff --git a/sysdeps/unix/sysv/linux/rename.c b/sysdeps/unix/sysv/linux/rename.c > > new file mode 100644 > > index 0000000..62a58ae > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/rename.c > > @@ -0,0 +1,34 @@ > > +/* rename() syscall > > + Copyright (C) 2016 Free Software Foundation, Inc. > > + This file is part of the GNU C Library. > > + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. > > + > > + 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 <stdio.h> > > +#include <unistd.h> > > +#include <fcntl.h> > > +#include <sysdep.h> > > + > > +/* Rename the file OLD to NEW. */ > > +int > > +rename (const char *old, const char *new) > > +{ > > +#ifdef __NR_renameat2 > > + return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); > > +#else > > + return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); > > +#endif > > That breaks all kernels that don't implement renameat2. If kernel doesn't implement renameat2, it also doesn't #define __NR_renameat2, and so #else branch of #ifdef condition will be chosen, which is exactly like it was workibg before. Or I miss something? > And it doesn't > compile: > > ../sysdeps/unix/sysv/linux/rename.c: In function ‘rename’: > ../sysdeps/unix/sysv/linux/rename.c:30:3: error: implicit declaration of function ‘__set_errno’ [-Werror=implicit-function-declaration] > return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); > ^ Sounds pretty weird. I checked again with aarch64, and it does work for both defined and undefined __NR_renameat2. In your log I see that the problem is not in __NR_renameat2 or INLINE_SYSCALL but in __set_errno which is undefined for some reason. In aarch64 INLINE_SYSCALL() is defined in platform sysdep.h, and that file includes errno.h with very verbose comment: /* In order to get __set_errno() definition in INLINE_SYSCALL. */ #ifndef __ASSEMBLER__ #include <errno.h> #endif I can #include <errno.h> explicitly, but I think sysdep.h should do it... What the platform fails the build for you? I also wonder how it works now, because current implementation is also based on INLINE_SYSCALL(). > > diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list > > index 7ae2541..a2c1060 100644 > > --- a/sysdeps/unix/sysv/linux/syscalls.list > > +++ b/sysdeps/unix/sysv/linux/syscalls.list > > @@ -89,7 +89,6 @@ fchownat - fchownat i:isiii fchownat > > linkat - linkat i:isisi linkat > > mkdirat - mkdirat i:isi mkdirat > > readlinkat - readlinkat i:issi readlinkat > > -renameat - renameat i:isis renameat > > symlinkat - symlinkat i:sis symlinkat > > unlinkat - unlinkat i:isi unlinkat > > The only other implementation of renameat is the stub in stdio-common > which always fails. OOPS. I misread renameat as rename. I can send v2 that introduces renameat.c, like rename.c in this patch, and rename() will just call renameat(); if we'll resolve build issue that you observe. Yury.
On Sat, Oct 15, 2016 at 10:55 AM, Yury Norov <ynorov@caviumnetworks.com> wrote: > On Sat, Oct 15, 2016 at 03:02:27PM +0200, Andreas Schwab wrote: >> On Okt 15 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: >> > +/* Rename the file OLD to NEW. */ >> > +int >> > +rename (const char *old, const char *new) >> > +{ >> > +#ifdef __NR_renameat2 >> > + return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); >> > +#else >> > + return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); >> > +#endif >> >> That breaks all kernels that don't implement renameat2. > > If kernel doesn't implement renameat2, it also doesn't > #define __NR_renameat2, and so #else branch of #ifdef condition > will be chosen, which is exactly like it was workibg before. Or > I miss something? It is very common to *compile* glibc against the very latest kernel headers but *run* it with an older kernel. Until the minimum *runtime* supported kernel is guaranteed to provide renameat2, this needs to be something like int rename (const char *old, const char *new) { #ifdef __ASSUME_RENAMEAT2 return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); #else # ifdef __NR_renameat2 static bool try_renameat2 = true; if (try_renameat2) { INTERNAL_SYSCALL_DECL (err); int ret = INTERNAL_SYSCALL (renameat2, err, 5, AT_FDCWD, old, AT_FDCWD, new, 0); if (!INTERNAL_SYSCALL_ERROR_P (ret, err)) return 0; int errnm = INTERNAL_SYSCALL_ERRNO (ret, err); if (errnm != ENOSYS) { __set_errno (errnm); return -1; } try_renameat2 = false; } # endif return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); #endif } with an appropriate conditional definition of __ASSUME_RENAMEAT2 added to kernel-features.h. (renameat2 appears to have been added in 3.15, which I *think* corresponds to __LINUX_KERNEL_VERSION >= 0x030f00.) > In aarch64 INLINE_SYSCALL() is > defined in platform sysdep.h, and that file includes errno.h with very > verbose comment: > > /* In order to get __set_errno() definition in INLINE_SYSCALL. */ > #ifndef __ASSEMBLER__ > #include <errno.h> > #endif > > I can #include <errno.h> explicitly, but I think sysdep.h should do it... I concur, if INLINE_SYSCALL/INTERNAL_SYSCALL have been defined, __set_errno should also be defined. >> The only other implementation of renameat is the stub in stdio-common >> which always fails. > > OOPS. I misread renameat as rename. I can send v2 that introduces > renameat.c, like rename.c in this patch, and rename() will just call > renameat(); if we'll resolve build issue that you observe. If our exposed renameat() has no flags argument, then this can be handled as above; otherwise it's going to need to check for flags != 0 in the no-renameat2 case and set errno to ENOSYS itself. zw
On Sat, 15 Oct 2016, Zack Weinberg wrote: > with an appropriate conditional definition of __ASSUME_RENAMEAT2 added > to kernel-features.h. (renameat2 appears to have been added in 3.15, > which I *think* corresponds to __LINUX_KERNEL_VERSION >= 0x030f00.) And it's necessary when adding such definitions to check the kernel for *all* architectures supported by glibc, to make sure the syscall is present in both asm/unistd.h and the syscall table on all such architectures, since sometimes a syscall is not added for all architectures at the same time, or isn't added to both the syscall table and unistd.h at the same time. The minimal conservative patch to handle architectures with only renameat2 is to make sysdeps/unix/sysv/linux/generic/rename.c use renameat if __NR_renameat is used, otherwise use renameat2 - *and* to add sysdeps/unix/sysv/linux/generic/renameat.c to implement renameat in terms of the renameat2 syscall. There is no need to change which syscalls are used for architectures where rename / renameat syscalls are available - thus, if you propose to do so, such a proposal would best be separated from fixing the case of architectures with only renameat2, and given its own self-contained rationale. In my view, the complexity of runtime tests for the renameat2 syscall is unjustified; implementing rename or renameat in terms of the renameat2 syscall on architectures with rename or renameat syscalls should not be considered until the minimum supported kernel for those architectures is one in which the renameat2 syscall is available.
On Sun, Oct 16, 2016 at 01:47:14PM +0000, Joseph Myers wrote: > There is no need to change which syscalls are used for architectures where > rename / renameat syscalls are available I agree, the original patch only affects new future architectures in mainline Linux (i.e. nobody at the moment, since the patch was too late to catch nios2). The old renameat syscall is not deprecated as such for any existing mainline architectures, it simply won't exist for any new ones. I.e. if anything the test should be about whether [neither rename or] renameat exists, not whether renameat2 exists. Cheers James
diff --git a/sysdeps/unix/sysv/linux/generic/rename.c b/sysdeps/unix/sysv/linux/generic/rename.c deleted file mode 100644 index 174c147..0000000 --- a/sysdeps/unix/sysv/linux/generic/rename.c +++ /dev/null @@ -1,29 +0,0 @@ -/* Copyright (C) 2011-2016 Free Software Foundation, Inc. - This file is part of the GNU C Library. - Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. - - 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 <stdio.h> -#include <unistd.h> -#include <fcntl.h> -#include <sysdep.h> - -/* Rename the file OLD to NEW. */ -int -rename (const char *old, const char *new) -{ - return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); -} diff --git a/sysdeps/unix/sysv/linux/rename.c b/sysdeps/unix/sysv/linux/rename.c new file mode 100644 index 0000000..62a58ae --- /dev/null +++ b/sysdeps/unix/sysv/linux/rename.c @@ -0,0 +1,34 @@ +/* rename() syscall + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + Contributed by Chris Metcalf <cmetcalf@tilera.com>, 2011. + + 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 <stdio.h> +#include <unistd.h> +#include <fcntl.h> +#include <sysdep.h> + +/* Rename the file OLD to NEW. */ +int +rename (const char *old, const char *new) +{ +#ifdef __NR_renameat2 + return INLINE_SYSCALL (renameat2, 5, AT_FDCWD, old, AT_FDCWD, new, 0); +#else + return INLINE_SYSCALL (renameat, 4, AT_FDCWD, old, AT_FDCWD, new); +#endif +} diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list index 7ae2541..a2c1060 100644 --- a/sysdeps/unix/sysv/linux/syscalls.list +++ b/sysdeps/unix/sysv/linux/syscalls.list @@ -89,7 +89,6 @@ fchownat - fchownat i:isiii fchownat linkat - linkat i:isisi linkat mkdirat - mkdirat i:isi mkdirat readlinkat - readlinkat i:issi readlinkat -renameat - renameat i:isis renameat symlinkat - symlinkat i:sis symlinkat unlinkat - unlinkat i:isi unlinkat
renameat syscall was deprecated in kernel in patch b0da6d44 (asm-generic: Drop renameat syscall from default list). But glibc is still refers it in rename(). This patch consolidates linux/ and linux/generic/ implementations of rename(), and makes it call sys_renameat2() if kernel exposes it. Tested on arm64 lp64 and ilp32. * sysdeps/unix/sysv/linux/generic/rename.c: Remove * sysdeps/unix/sysv/linux/rename.c: New file. * sysdeps/unix/sysv/linux/syscalls.list: Drop renameat. Signed-off-by: Yury Norov <ynorov@caviumnetworks.com> --- sysdeps/unix/sysv/linux/generic/rename.c | 29 --------------------------- sysdeps/unix/sysv/linux/rename.c | 34 ++++++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/syscalls.list | 1 - 3 files changed, 34 insertions(+), 30 deletions(-) delete mode 100644 sysdeps/unix/sysv/linux/generic/rename.c create mode 100644 sysdeps/unix/sysv/linux/rename.c