Re: [PATCH] Linux: consolidate rename()

[Yury Norov <ynorov at caviumnetworks dot com>]

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.