[PATCH v2] Linux: consolidate rename()

[PATCH v2] Linux: consolidate rename()

[Yury Norov]

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

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
 
-- 
2.7.4

Re: [PATCH v2] Linux: consolidate rename()

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v2] Linux: consolidate rename()

[Yury Norov]

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

Re: [PATCH v2] Linux: consolidate rename()

[Andreas Schwab]

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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v2] Linux: consolidate rename()

[Yury Norov]

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?

Re: [PATCH v2] Linux: consolidate rename()

[Joseph Myers]

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.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2] Linux: consolidate rename()

[Adhemerval Zanella]

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).

Re: [PATCH v2] Linux: consolidate rename()

[Joseph Myers]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH v2] Linux: consolidate rename()

[Adhemerval Zanella]

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).