From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81015 invoked by alias); 1 Jul 2018 21:49:46 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 81006 invoked by uid 89); 1 Jul 2018 21:49:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=BAYES_20,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=page, miss, definitely, preserve X-HELO: NAM03-CO1-obe.outbound.protection.outlook.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=CAVIUMNETWORKS.onmicrosoft.com; s=selector1-cavium-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=1mRumJs3URZTPNjo0IFommuE6DYZHpxKXmCAOhIbXXc=; b=FqQNNxd0D6LHm+4kbBy+vYBGp6C71dp0Idk6KGgjBnHDRhUs+e93bR9laCgnTPzcLq93204qVZiKWOPbnu7xwjGTgoded+W59cbcgenKzi3ui8fhPbYFrcxVrgZAHxy+x2bZIZNoKx70lUxf1+5AXhunbWBu/l2okKaJ9p5B2wY= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Yuri.Norov@cavium.com; Date: Sun, 01 Jul 2018 21:49:00 -0000 From: Yury Norov To: Florian Weimer Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] Add renameat2 function [BZ #17662] Message-ID: <20180701214901.GA32498@yury-thinkpad> References: <20180630121447.E4C8643994575@oldenburg.str.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180630121447.E4C8643994575@oldenburg.str.redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) Return-Path: ynorov@caviumnetworks.com Received-SPF: None (protection.outlook.com: cavium.com does not designate permitted sender hosts) X-SW-Source: 2018-07/txt/msg00001.txt.bz2 Hi Florian, On Sat, Jun 30, 2018 at 02:14:47PM +0200, Florian Weimer wrote: > The implementation falls back to renameat if renameat2 is not available > in the kernel (or in the kernel headers) and the flags argument is zero. > Without kernel support, a non-zero argument returns EINVAL, not ENOSYS. > This mirrors what the kernel does for invalid renameat2 flags. > > 2018-06-30 Florian Weimer > > [BZ # 17662] > * libio/stdio.h [__USE_GNU] (RENAME_NOREPLACE, RENAME_EXCHANGE) > (RENAME_WHITEOUT): Define. > [__USE_GNU] (renameat2): Declare. > * stdio-common/Makefile (routines): Add renameat2. > (tests): Add tst-renameat2. > * stdio-common/Versions (GLIBC_2_28): Export renameat2. > * stdio-common/renameat2.c: New file. > * stdio-common/tst-renameat2.c: Likewise. > * sysdeps/unix/sysv/linux/renameat2.c: Likewise. > * manual/filesys.texi (Temporary Files): Note that renameat2 is > undocumented. > * sysdeps/unix/sysv/linux/kernel-features.h > [__LINUX_KERNEL_VERSION >= 0x030F00] (__ASSUME_RENAMEAT2): Define. > * include/stdio.h (__renameat): Add alias for renameat. > * stdio-common/renameat.c (__renameat): Rename from renameat. > Add hidden definition and alias. > * sysdeps/unix/sysv/linux/renameat.c: Likewise. > * sysdeps/mach/hurd/renameat.c: Likewise. > * sysdeps/**/libc*.abilist: Add renameat2. > > diff --git a/NEWS b/NEWS > index a27dd371a3..093f364c7e 100644 > --- a/NEWS > +++ b/NEWS > @@ -35,6 +35,8 @@ Major new features: > * Building and running on GNU/Hurd systems now works without out-of-tree > patches. > > +* The renameat2 function has been added. > + > * IDN domain names in getaddrinfo and getnameinfo now use the system libidn2 > library if installed. libidn2 version 2.0.5 or later is recommended. If > libidn2 is not available, internationalized domain names are not encoded > diff --git a/include/stdio.h b/include/stdio.h > index f140813ad6..3ba0edc924 100644 > --- a/include/stdio.h > +++ b/include/stdio.h > @@ -237,5 +237,8 @@ __putc_unlocked (int __c, FILE *__stream) > } > # endif > > +extern __typeof (renameat) __renameat; > +libc_hidden_proto (__renameat) > + > # endif /* not _ISOMAC */ > #endif /* stdio.h */ > diff --git a/libio/stdio.h b/libio/stdio.h > index 731f8e56f4..3cdc8cc532 100644 > --- a/libio/stdio.h > +++ b/libio/stdio.h > @@ -153,6 +153,18 @@ extern int renameat (int __oldfd, const char *__old, int __newfd, > const char *__new) __THROW; > #endif > > +#ifdef __USE_GNU > +/* Flags for renameat. */ Flags for renameat2, right? > +# 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. This is quite common though and sometimes causes real troubles. sysdeps/unix/sysv/linux/s390/tst-ptrace-singleblock.c > + > +/* Rename file OLD relative to OLDFD to NEW relative to NEWFD, with > + additional flags. */ > +extern int renameat2 (int __oldfd, const char *__old, int __newfd, > + const char *__new, unsigned int __flags) __THROW; > +#endif > + > /* Create a temporary file and open it read/write. > > This function is a possible cancellation point and therefore not > diff --git a/manual/filesys.texi b/manual/filesys.texi > index cc70a6b7ee..db2f1041de 100644 > --- a/manual/filesys.texi > +++ b/manual/filesys.texi > @@ -3552,6 +3552,7 @@ The @code{mkdtemp} function comes from OpenBSD. > @c open_by_handle_at > @c readlinkat > @c renameat > +@c renameat2 > @c scandirat > @c symlinkat > @c unlinkat > diff --git a/stdio-common/Makefile b/stdio-common/Makefile > index 738a3cead0..6fd628708f 100644 > --- a/stdio-common/Makefile > +++ b/stdio-common/Makefile > @@ -35,7 +35,7 @@ routines := \ > perror psignal \ > tmpfile tmpfile64 tmpnam tmpnam_r tempnam tempname \ > getline getw putw \ > - remove rename renameat \ > + remove rename renameat renameat2 \ > flockfile ftrylockfile funlockfile \ > isoc99_scanf isoc99_vscanf isoc99_fscanf isoc99_vfscanf isoc99_sscanf \ > isoc99_vsscanf \ > @@ -62,6 +62,7 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \ > tst-vfprintf-user-type \ > tst-vfprintf-mbs-prec \ > tst-scanf-round \ > + tst-renameat2 \ > > test-srcs = tst-unbputc tst-printf > > diff --git a/stdio-common/Versions b/stdio-common/Versions > index 5016f69c20..b8217578c8 100644 > --- a/stdio-common/Versions > +++ b/stdio-common/Versions > @@ -57,6 +57,9 @@ libc { > psiginfo; > register_printf_modifier; register_printf_type; register_printf_specifier; > } > + GLIBC_2.28 { > + renameat2; > + } > GLIBC_PRIVATE { > # global variables > _itoa_lower_digits; > 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? > diff --git a/stdio-common/renameat2.c b/stdio-common/renameat2.c > new file mode 100644 > index 0000000000..c2cedcd2cb > --- /dev/null > +++ b/stdio-common/renameat2.c > @@ -0,0 +1,30 @@ > +/* Generic implementation of the renameat function. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + 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 > + . */ > + > +#include > +#include > + > +int > +renameat2 (int oldfd, const char *old, int newfd, const char *new, > + unsigned int flags) > +{ > + if (flags == 0) > + return __renameat (oldfd, old, newfd, new); Please, empty line after 'return ...'. 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. Also, if, say, flags is 0 and oldfd < 0, the error will be EBADF; but if flags != 0 and oldfd < 0, the error will be EINVAL. This is definitely the mess that I'd like to avoid in system library. Why not just drop this 2 lines? > + __set_errno (EINVAL); It should be ENOSYS, I suppose. > + return -1; > +} > diff --git a/stdio-common/tst-renameat2.c b/stdio-common/tst-renameat2.c > new file mode 100644 > index 0000000000..958b0918d6 > --- /dev/null > +++ b/stdio-common/tst-renameat2.c > @@ -0,0 +1,204 @@ > +/* Linux implementation for renameat2 function. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + 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 > + . */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* Directory with the temporary files. */ > +static char *directory; > +static int directory_fd; > + > +/* Paths within that directory. */ > +static char *old_path; /* File is called "old". */ > +static char *new_path; /* File is called "new". */ > + > +/* Subdirectory within the directory above. */ > +static char *subdirectory; > +int subdirectory_fd; > + > +/* And a pathname in that directory (called "file"). */ > +static char *subdir_path; > + > +static void > +prepare (int argc, char **argv) > +{ > + directory = support_create_temp_directory ("tst-renameat2-"); > + directory_fd = xopen (directory, O_RDONLY | O_DIRECTORY, 0); > + old_path = xasprintf ("%s/old", directory); > + add_temp_file (old_path); > + new_path = xasprintf ("%s/new", directory); > + add_temp_file (new_path); > + subdirectory = xasprintf ("%s/subdir", directory); > + xmkdir (subdirectory, 0777); > + add_temp_file (subdirectory); > + subdirectory_fd = xopen (subdirectory, O_RDONLY | O_DIRECTORY, 0); > + subdir_path = xasprintf ("%s/file", subdirectory); > + add_temp_file (subdir_path); > +} > + > +/* Delete all files, preparing a clean slate for the next test. */ > +static void > +delete_all_files (void) > +{ > + char *files[] = { old_path, new_path, subdir_path }; > + for (size_t i = 0; i < array_length (files); ++i) > + if (unlink (files[i]) != 0 && errno != ENOENT) > + FAIL_EXIT1 ("unlink (\"%s\"): %m", files[i]); > +} > + > +/* Return true if PATH exists in the file system. */ > +static bool > +file_exists (const char *path) > +{ > + return access (path, F_OK) == 0; > +} > + > +/* Check that PATH exists and has size EXPECTED_SIZE. */ > +static void > +check_size (const char *path, off64_t expected_size) > +{ > + struct stat64 st; > + xstat (path, &st); > + if (st.st_size != expected_size) > + FAIL_EXIT1 ("file \"%s\": expected size %lld, actual size %lld", > + path, (unsigned long long int) expected_size, > + (unsigned long long int) st.st_size); > +} > + > +/* Rename tests where the target does not exist. */ > +static void > +rename_without_existing_target (unsigned int flags) > +{ > + delete_all_files (); > + support_write_file_string (old_path, ""); > + TEST_COMPARE (renameat2 (AT_FDCWD, old_path, AT_FDCWD, new_path, flags), 0); > + TEST_VERIFY (!file_exists (old_path)); > + TEST_VERIFY (file_exists (new_path)); > + > + delete_all_files (); > + support_write_file_string (old_path, ""); > + TEST_COMPARE (renameat2 (directory_fd, "old", AT_FDCWD, new_path, flags), 0); > + TEST_VERIFY (!file_exists (old_path)); > + TEST_VERIFY (file_exists (new_path)); > + > + delete_all_files (); > + support_write_file_string (old_path, ""); > + TEST_COMPARE (renameat2 (directory_fd, "old", subdirectory_fd, "file", 0), > + 0); > + TEST_VERIFY (!file_exists (old_path)); > + TEST_VERIFY (file_exists (subdir_path)); > +} > + > +static int > +do_test (void) > +{ > + /* Tests with zero flags argument. These are expected to succeed > + because this renameat2 variant can be implemented with > + renameat. */ > + rename_without_existing_target (0); > + > + /* renameat2 without flags replaces an existing destination. */ > + delete_all_files (); > + support_write_file_string (old_path, "123"); > + support_write_file_string (new_path, "1234"); > + TEST_COMPARE (renameat2 (AT_FDCWD, old_path, AT_FDCWD, new_path, 0), 0); > + TEST_VERIFY (!file_exists (old_path)); > + check_size (new_path, 3); > + > + /* 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". According to man page, for renameat2 EINVAL means one of: EINVAL An invalid flag was specified in flags. EINVAL Both RENAME_NOREPLACE and RENAME_EXCHANGE were specified in flags. EINVAL Both RENAME_WHITEOUT and RENAME_EXCHANGE were specified in flags. EINVAL The filesystem does not support one of the flags in flags. > + else > + FAIL_EXIT1 ("renameat2 probe failed: %m"); > + } > + else > + { > + /* We have full renameat2 support. */ > + rename_without_existing_target (RENAME_NOREPLACE); > + > + /* Now test RENAME_NOREPLACE with an existing target. */ > + delete_all_files (); > + support_write_file_string (old_path, "123"); > + support_write_file_string (new_path, "1234"); > + TEST_COMPARE (renameat2 (AT_FDCWD, old_path, AT_FDCWD, new_path, > + RENAME_NOREPLACE), -1); > + TEST_COMPARE (errno, EEXIST); > + check_size (old_path, 3); > + check_size (new_path, 4); > + > + delete_all_files (); > + support_write_file_string (old_path, "123"); > + support_write_file_string (new_path, "1234"); > + TEST_COMPARE (renameat2 (directory_fd, "old", AT_FDCWD, new_path, > + RENAME_NOREPLACE), -1); > + TEST_COMPARE (errno, EEXIST); > + check_size (old_path, 3); > + check_size (new_path, 4); > + > + delete_all_files (); > + support_write_file_string (old_path, "123"); > + support_write_file_string (subdir_path, "1234"); > + TEST_COMPARE (renameat2 (directory_fd, "old", subdirectory_fd, "file", > + RENAME_NOREPLACE), -1); > + TEST_COMPARE (errno, EEXIST); > + check_size (old_path, 3); > + check_size (subdir_path, 4); > + > + /* The flag combination of RENAME_NOREPLACE and RENAME_EXCHANGE > + is invalid. */ > + TEST_COMPARE (renameat2 (directory_fd, "ignored", > + subdirectory_fd, "ignored", > + RENAME_NOREPLACE | RENAME_EXCHANGE), -1); > + TEST_COMPARE (errno, EINVAL); > + } > + > + /* Create all the pathnames to avoid warnings from the test > + harness. */ > + support_write_file_string (old_path, ""); > + support_write_file_string (new_path, ""); > + support_write_file_string (subdir_path, ""); > + > + free (directory); > + free (subdirectory); > + free (old_path); > + free (new_path); > + free (subdir_path); > + > + xclose (directory_fd); > + xclose (subdirectory_fd); > + > + return 0; > +} > + > +#define PREPARE prepare > +#include [...] > diff --git a/sysdeps/unix/sysv/linux/renameat.c b/sysdeps/unix/sysv/linux/renameat.c > index 034432b934..f85c5ae0ec 100644 > --- a/sysdeps/unix/sysv/linux/renameat.c > +++ b/sysdeps/unix/sysv/linux/renameat.c > @@ -22,7 +22,7 @@ > #include > > int > -renameat (int oldfd, const char *old, int newfd, const char *new) > +__renameat (int oldfd, const char *old, int newfd, const char *new) > { > #ifdef __NR_renameat > return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); > @@ -30,3 +30,5 @@ renameat (int oldfd, const char *old, int newfd, const char *new) > return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, 0); > #endif > } > +libc_hidden_def (__renameat) > +weak_alias (__renameat, renameat) > diff --git a/sysdeps/unix/sysv/linux/renameat2.c b/sysdeps/unix/sysv/linux/renameat2.c > new file mode 100644 > index 0000000000..919bb2a0d4 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/renameat2.c > @@ -0,0 +1,44 @@ > +/* Linux implementation for renameat2 function. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + 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 > + . */ > + > +#include > +#include > +#include > + > +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. > +# ifdef __NR_renameat2 > + /* For non-zero flags, try the renameat2 system call. */ > + int ret = INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, flags); > + if (ret != -1 || errno != ENOSYS) > + /* Preserve non-error/non-ENOSYS return values. */ > + return ret; You can drop if (...), just return INLINE_SYSCALL_CALL(...). > +# endif > + /* No kernel (header) support for renameat2. All flags are > + unknown. */ > + __set_errno (EINVAL); You really need ENOSYS here. > + return -1; > +#endif > +} I think this function should look like this (not tested): int renameat2 (int oldfd, const char *old, int newfd, const char *new, unsigned int flags) { # ifdef __NR_renameat2 return INLINE_SYSCALL_CALL (renameat2, oldfd, old, newfd, new, flags); # else /* Try renameat if possible. */ if (flags == 0) return INLINE_SYSCALL_CALL (renameat, oldfd, old, newfd, new); __set_errno (ENOSYS); return -1; # endif } Yury