Message ID | 1476924756-31448-1-git-send-email-ynorov@caviumnetworks.com |
---|---|
State | New |
Headers | show |
On Okt 20 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..8e14d7d > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/rename.c This overrides the unix/syscalls.list entry, thus architectures which implement the rename syscall no longer use it. > @@ -0,0 +1,27 @@ > +/* rename() implemantation. > + 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 <fcntl.h> > + > +/* Rename the file OLD to NEW. */ > +int > +rename (const char *old, const char *new) > +{ > + return renameat (AT_FDCWD, old, AT_FDCWD, new); > +} ../sysdeps/unix/sysv/linux/rename.c: In function ‘rename’: ../sysdeps/unix/sysv/linux/rename.c:26:3: error: implicit declaration of function ‘renameat’ [-Werror=implicit-function-declaration] return renameat (AT_FDCWD, old, AT_FDCWD, new); ^ Andreas.
Hi Andreas, On Thu, Oct 20, 2016 at 09:10:48AM +0200, Andreas Schwab wrote: > On Okt 20 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..8e14d7d > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/rename.c > > This overrides the unix/syscalls.list entry, thus architectures which > implement the rename syscall no longer use it. Yes. You want me to notice it in commit message? > > @@ -0,0 +1,27 @@ > > +/* rename() implemantation. > > + 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 <fcntl.h> > > + > > +/* Rename the file OLD to NEW. */ > > +int > > +rename (const char *old, const char *new) > > +{ > > + return renameat (AT_FDCWD, old, AT_FDCWD, new); > > +} > > ../sysdeps/unix/sysv/linux/rename.c: In function ‘rename’: > ../sysdeps/unix/sysv/linux/rename.c:26:3: error: implicit declaration of function ‘renameat’ [-Werror=implicit-function-declaration] > return renameat (AT_FDCWD, old, AT_FDCWD, new); > ^ Yes. The file should #inclide <stdio.h> to fix it. If no other issues, I'll send v3 soon. Yury
On Okt 20 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: > Hi Andreas, > > On Thu, Oct 20, 2016 at 09:10:48AM +0200, Andreas Schwab wrote: >> On Okt 20 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..8e14d7d >> > --- /dev/null >> > +++ b/sysdeps/unix/sysv/linux/rename.c >> >> This overrides the unix/syscalls.list entry, thus architectures which >> implement the rename syscall no longer use it. > > Yes. You want me to notice it in commit message? Do all supported kernels implement renameat? Andreas.
On Thu, Oct 20, 2016 at 02:35:05PM +0200, Andreas Schwab wrote: > On Okt 20 2016, Yury Norov <ynorov@caviumnetworks.com> wrote: > > > Hi Andreas, > > > > On Thu, Oct 20, 2016 at 09:10:48AM +0200, Andreas Schwab wrote: > >> On Okt 20 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..8e14d7d > >> > --- /dev/null > >> > +++ b/sysdeps/unix/sysv/linux/rename.c > >> > >> This overrides the unix/syscalls.list entry, thus architectures which > >> implement the rename syscall no longer use it. > > > > Yes. You want me to notice it in commit message? > > Do all supported kernels implement renameat? Ah, now I understand you. It should be then like #include <fcntl.h> #include <stdio.h> /* Rename the file OLD to NEW. */ int rename (const char *old, const char *new) { #ifdef __NR_rename return INLINE_SYSCALL_CALL (rename, old, new); #else return renameat (AT_FDCWD, old, AT_FDCWD, new); #endif } Is it OK?
On Thu, 20 Oct 2016, Yury Norov wrote: > Tested on arm64/lp64 which supports renameat and renameat2, > and aarch64/ilp32 which supports renameat2 only. There's clearly something very wrong with your test environment that you need to resolve before submitting any more glibc patches at all. If you'd tested this patch you should have seen linknamespace tests failing because rename is in standards that do not include renameat, and the localplt test failing because you're calling an exported function that isn't using the libc_hidden_proto/libc_hidden_def machinery (even non-exported functions have to use that machinery when called within the same library, but failure to do so is less visible from test results). Just leave rename.c in sysdeps/unix/sysv/linux/generic and add a case for it to use the renameat2 syscall and you avoid all these issues; on platforms with the rename syscall, there is no benefit from going via other syscalls, and calling renameat from rename rather than using syscalls introduces unnecessary complications. (But you must still test by running the full glibc testsuite and actually investigating any failures seen.)
On 20/10/2016 11:13, Joseph Myers wrote: > On Thu, 20 Oct 2016, Yury Norov wrote: > >> Tested on arm64/lp64 which supports renameat and renameat2, >> and aarch64/ilp32 which supports renameat2 only. > > There's clearly something very wrong with your test environment that you > need to resolve before submitting any more glibc patches at all. If you'd > tested this patch you should have seen linknamespace tests failing because > rename is in standards that do not include renameat, and the localplt test > failing because you're calling an exported function that isn't using the > libc_hidden_proto/libc_hidden_def machinery (even non-exported functions > have to use that machinery when called within the same library, but > failure to do so is less visible from test results). > > Just leave rename.c in sysdeps/unix/sysv/linux/generic and add a case for > it to use the renameat2 syscall and you avoid all these issues; on > platforms with the rename syscall, there is no benefit from going via > other syscalls, and calling renameat from rename rather than using > syscalls introduces unnecessary complications. (But you must still test > by running the full glibc testsuite and actually investigating any > failures seen.) > I think the idea is to remove the 'generic' folder and let the sysdeps/unix/sysv/linux/ be the generic Linux implementation (which makes more sense IMHO and avoid confusion in which folder to imply). Yury, as Joseph is pointing out you need to run full make check from GLIBC to check not only that syscall is running but also if linkspace is being clean and inside PLT is what to expected. After make check shows no regression you can move forward to functionality tests like LTP. Also, I would recommend to run a sanity make check on x86 as well. Now, related to patch I think a simpler implementation for both rename and renameat would be just: * sysdeps/unix/sysv/linux/rename.c: int rename (const char *old, const char *new) { #ifdef __NR_rename return INLINE_SYSCALL_CALL (rename, old, new); #else return INLINE_SYSCALL_CALL (renameat, AT_FDCWD, old, AT_FDCWD, new); #endif } * sysdeps/unix/sysv/linux/renameat.c int renameat (int oldfd, const char *old, int newfd, const char *new) { #ifdef __NR_renameat return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); #else return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, 0); #endif } I think we can drop the #error on renameat since at least __NR_renameat should be defined in all supported kernels (it was added on 2.6.16 afaik).
On Thu, 20 Oct 2016, Adhemerval Zanella wrote: > I think the idea is to remove the 'generic' folder and let the > sysdeps/unix/sysv/linux/ be the generic Linux implementation (which makes > more sense IMHO and avoid confusion in which folder to imply). That's fine, but when there are so many deficiencies in a single patch it seems to make sense to suggest it be split into baby steps, so supporting systems with only renameat2 is separate from unifying the implementations of these functions into a single set of sources, which is separate from any change of the rename function from using the rename syscall to using the renameat syscall on platforms that have the rename syscall (if such a change is desired at all). > Now, related to patch I think a simpler implementation for both > rename and renameat would be just: > > * sysdeps/unix/sysv/linux/rename.c: > > int > rename (const char *old, const char *new) > { > #ifdef __NR_rename > return INLINE_SYSCALL_CALL (rename, old, new); > #else > return INLINE_SYSCALL_CALL (renameat, AT_FDCWD, old, AT_FDCWD, new); > #endif > } That of course is an implementation for the present situation of not handling renameat2-only architectures, and a further conditional would be needed for handling such architectures (but with a division into baby steps, supporting renameat2 here would be separate from combining the support for rename and renameat syscalls).
On 20/10/2016 13:31, Joseph Myers wrote: > On Thu, 20 Oct 2016, Adhemerval Zanella wrote: > >> I think the idea is to remove the 'generic' folder and let the >> sysdeps/unix/sysv/linux/ be the generic Linux implementation (which makes >> more sense IMHO and avoid confusion in which folder to imply). > > That's fine, but when there are so many deficiencies in a single patch it > seems to make sense to suggest it be split into baby steps, so supporting > systems with only renameat2 is separate from unifying the implementations > of these functions into a single set of sources, which is separate from > any change of the rename function from using the rename syscall to using > the renameat syscall on platforms that have the rename syscall (if such a > change is desired at all). Yes, for this specific patch it seems sensible to split it in smaller changes. > >> Now, related to patch I think a simpler implementation for both >> rename and renameat would be just: >> >> * sysdeps/unix/sysv/linux/rename.c: >> >> int >> rename (const char *old, const char *new) >> { >> #ifdef __NR_rename >> return INLINE_SYSCALL_CALL (rename, old, new); >> #else >> return INLINE_SYSCALL_CALL (renameat, AT_FDCWD, old, AT_FDCWD, new); >> #endif >> } > > That of course is an implementation for the present situation of not > handling renameat2-only architectures, and a further conditional would be > needed for handling such architectures (but with a division into baby > steps, supporting renameat2 here would be separate from combining the > support for rename and renameat syscalls). Indeed I did not take this constraint in consideration (an architecture that only supports renameat2).
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..8e14d7d --- /dev/null +++ b/sysdeps/unix/sysv/linux/rename.c @@ -0,0 +1,27 @@ +/* rename() implemantation. + 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 <fcntl.h> + +/* Rename the file OLD to NEW. */ +int +rename (const char *old, const char *new) +{ + return renameat (AT_FDCWD, old, AT_FDCWD, new); +} diff --git a/sysdeps/unix/sysv/linux/renameat.c b/sysdeps/unix/sysv/linux/renameat.c new file mode 100644 index 0000000..a735e2d --- /dev/null +++ b/sysdeps/unix/sysv/linux/renameat.c @@ -0,0 +1,38 @@ +/* renameat() 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> + +#if !defined (__NR_renameat) && !defined (__NR_renameat2) +# error "Both renameat and renameat2 not supported" +#endif + +/* Rename the file OLD to NEW. */ +int +renameat (int oldfd, const char *old, int newfd, const char *new) +{ +#ifdef __NR_renameat + return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); +#else + return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, 0); +#endif +} diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list index 68af897..4d7d71e 100644 --- a/sysdeps/unix/sysv/linux/syscalls.list +++ b/sysdeps/unix/sysv/linux/syscalls.list @@ -88,7 +88,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). In this patch linux/ and linux/generic/ implementations of rename() and renameat() are consolidated and forced to call renameat2 if renameat is not exposed by kernel headers. The change affects only new ports. Old ports work as usual. Tested on arm64/lp64 which supports renameat and renameat2, and aarch64/ilp32 which supports renameat2 only. Kernel and glibc series for aarch64/ilp32: https://github.com/norov/linux/commits/ilp32-4.9 https://github.com/norov/glibc/commits/master-ilp32 v2: - rename() calls renameat() now; - renameat() is implemented in a file, not in syscalls.list; - __NR_renameat2 is used if kernel doesn't define __NR_renameat, which is the case for new platforms, currently only aarch64/ilp32; * sysdeps/unix/sysv/linux/generic/rename.c: Remove. * sysdeps/unix/sysv/linux/rename.c: New file. * sysdeps/unix/sysv/linux/renameat.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 | 27 +++++++++++++++++++++++ sysdeps/unix/sysv/linux/renameat.c | 38 ++++++++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/syscalls.list | 1 - 4 files changed, 65 insertions(+), 30 deletions(-) delete mode 100644 sysdeps/unix/sysv/linux/generic/rename.c create mode 100644 sysdeps/unix/sysv/linux/rename.c create mode 100644 sysdeps/unix/sysv/linux/renameat.c