Re: [PATCH] Add renameat2 function [BZ #17662]

[Florian Weimer <fweimer at redhat dot com>]

On 07/01/2018 11:49 PM, Yury Norov wrote:

> > +#ifdef __USE_GNU
> > +/* Flags for renameat.  */
>
> Flags for renameat2, right?

Thanks, fixed.

> > +# define RENAME_NOREPLACE (1 << 0)
> > +# define RENAME_EXCHANGE (1 << 1)
> > +# define RENAME_WHITEOUT (1 << 2)
>
> I really don't understand how it works. Could you / somebody explain me?
>
> include/uapi/linux/fs.h in kernel sources already defines this flags,
> and this file is usually available in Linux distribution. So I don't
> understand what for it is duplicated here. If you keep in mind
> old linux headers or non-linux systems, I think it should be protected
> with #ifndef guards.

<linux/fs.h> undefines and defines macros not mentioned in the standards 
(and it even contains a few unrelated structs), so we cannot include it 
without _GNU_SOURCE.

It might be possible to include it only for _GNU_SOURCE, but there are a 
lot of things in <linux/fs.h>, so that does not seem to be particularly 
advisable.

We still support building glibc with 3.2 kernel headers, and if the 
definitions you quoted above are not available, building the test case 
would fail.

> > diff --git a/stdio-common/renameat.c b/stdio-common/renameat.c
> > index 2180b87bdf..98c8f1d18b 100644
> > --- a/stdio-common/renameat.c
> > +++ b/stdio-common/renameat.c
> > @@ -22,7 +22,7 @@
> >
> >   /* Rename the file OLD relative to OLDFD to NEW relative to NEWFD.  */
> >   int
> > -renameat (int oldfd, const char *old, int newfd, const char *new)
> > +__renameat (int oldfd, const char *old, int newfd, const char *new)
> >   {
> >     if ((oldfd < 0 && oldfd != AT_FDCWD) || (newfd < 0 && newfd != AT_FDCWD))
> >       {
> > @@ -40,5 +40,6 @@ renameat (int oldfd, const char *old, int newfd, const char *new)
> >     return -1;
> >   }
> >
> > -
> > +libc_hidden_def (__renameat)
> > +weak_alias (__renameat, renameat)
> >   stub_warning (renameat)
>
> This will introduce new function that will never be called if glibc is
> compiled against modern headers, and linker will not be able to throw
> it away. Or I miss something?

It will never be called on Linux or Hurd, no matter what the header 
versions are.  We have a policy that syscall wrappers need fallback 
definitions, and the fallback definitions are often never used (if both 
Hurd and Linux have implementations).

> > +int
> > +renameat2 (int oldfd, const char *old, int newfd, const char *new,
> > +           unsigned int flags)
> > +{
> > +  if (flags == 0)
> > +    return __renameat (oldfd, old, newfd, new);

> IIUC, you call __renameat here to make sanity check of arguments.
> For me as user of API, it would be more important to know that my
> system doesn't support syscall, than some argument is invalid (which
> makes me think that syscall is supported). It may waste my time
> as I will dig the problem in wrong direction.

No, the line above calls the Hurd or Linux renameat implementation, 
which are fully functional.

> > +  __set_errno (EINVAL);
>
> It should be ENOSYS, I suppose.

The kernel returns EINVAL for invalid flags.  If renameat2 is not 
implemented, all flags are invalid.

> > +  /* Now we need to check for kernel support of renameat2 with
> > +     flags.  */
> > +  delete_all_files ();
> > +  support_write_file_string (old_path, "");
> > +  if (renameat2 (AT_FDCWD, old_path, AT_FDCWD, new_path, RENAME_NOREPLACE)
> > +      != 0)
> > +    {
> > +      if (errno == EINVAL)
> > +        puts ("warning: no support for renameat2 with flags");
>
> This is wrong. There's no "renameat2 without flags".

I can change it to “renameat2 with non-zero flags”.

> > +int
> > +renameat2 (int oldfd, const char *old, int newfd, const char *new,
> > +           unsigned int flags)
> > +{
> > +#if !defined (__NR_renameat) || defined (__ASSUME_RENAMEAT2)
> > +  return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, flags);
> > +#else
> > +  if (flags == 0)
> > +    return __renameat (oldfd, old, newfd, new);
>
> What for this special case? The most-conservative-strategy argument
> doesn't work here because this is new syscall, and user really wants
> renameat2(), if he calls it explicitly in his new code.

That is not clear to me.  We do not cause renameat to fail if the system 
does not have the system call, but implements renameat2.  Why shouldn't 
we do the same thing for renameat2?

Thanks,
Florian