Re: [PATCH] Linux: consolidate rename()

[Zack Weinberg <zackw at panix dot com>]

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