This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

+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”.

+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);
+  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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]