[PATCH] Linux: consolidate rename()

[PATCH] Linux: consolidate rename()

[Yury Norov]

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

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

Re: [PATCH] Linux: consolidate rename()

[Andreas Schwab]

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.

-- 
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] Linux: consolidate rename()

[Yury Norov]

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.

Re: [PATCH] Linux: consolidate rename()

[Zack Weinberg]

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

Re: [PATCH] Linux: consolidate rename()

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Linux: consolidate rename()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]