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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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