renameat2
Ken Brown
kbrown@cornell.edu
Sat Aug 19 16:28:00 GMT 2017
Hi Corinna,
On 8/18/2017 11:15 AM, Corinna Vinschen wrote:
> Hi Ken,
>
> On Aug 18 09:21, Ken Brown wrote:
>> Linux has a system call 'renameat2' which is like renameat but has an
>> extra 'flags' argument. In particular, one can pass the
>> RENAME_NOREPLACE flag to cause the rename to fail with EEXIST if the
>> target of the rename exists. See
>>
>> http://man7.org/linux/man-pages/man2/rename.2.html
>>
>> macOS has a similar functionality, provided by the function
>> 'renameatx_np' with the flag RENAME_EXCL.
>>
>> There's also a recently introduced Gnulib module 'renameat2', but it
>> requires two system calls on Cygwin (one to test existence and the
>> second to do the rename), so that there is a race condition. On Linux
>> and macOS it uses renameat2 and renameatx_np to avoid the race.
>>
>> The attached patch implements renameat2 on Cygwin (but only supporting
>> the RENAME_NOREPLACE flag). I've written it so that a rename that
>> just changes case on a case-insensitive file system succeeds.
>>
>> If the patch is accepted, I'll submit a second patch that documents
>> the new function.
>
> Neat stuff, but there are a few points for discussion, see below.
>
>> --- a/winsup/cygwin/include/cygwin/fs.h
>> +++ b/winsup/cygwin/include/cygwin/fs.h
>> @@ -19,4 +19,9 @@ details. */
>> #define BLKPBSZGET 0x0000127b
>> #define BLKGETSIZE64 0x00041268
>>
>> +/* Flags for renameat2, from /usr/include/linux/fs.h. */
>> +#define RENAME_NOREPLACE (1 << 0)
>> +#define RENAME_EXCHANGE (1 << 1)
>> +#define RENAME_WHITEOUT (1 << 2)
>
> Given that there's no standard for this call (yet), do we really want to
> expose flag values we don't support? I would opt for only RENAME_NOREPLACE
> for now and skip the others.
>
>> +
>> #endif
>> diff --git a/winsup/cygwin/include/cygwin/version.h b/winsup/cygwin/include/cygwin/version.h
>> index efd4ac017..7640abfad 100644
>> --- a/winsup/cygwin/include/cygwin/version.h
>> +++ b/winsup/cygwin/include/cygwin/version.h
>> @@ -481,12 +481,14 @@ details. */
>> 314: Export explicit_bzero.
>> 315: Export pthread_mutex_timedlock.
>> 316: Export pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock.
>> + 317: Export renameat2. Add RENAME_NOREPLACE, RENAME_EXCHANGE,
>> + RENAME_WHITEOUT.
>
> You can drop the flag values here. renameat2 is sufficient.
>
>> +rename2 (const char *oldpath, const char *newpath, unsigned int flags)
>> {
>> tmp_pathbuf tp;
>> int res = -1;
>> @@ -2068,6 +2073,12 @@ rename (const char *oldpath, const char *newpath)
>>
>> __try
>> {
>> + if (flags & ~RENAME_NOREPLACE)
>> + /* RENAME_NOREPLACE is the only flag currently supported. */
>> + {
>> + set_errno (ENOTSUP);
>
> That should ideally be EINVAL. Unsupported bit values in a flag argument?
> EINVAL, please.
>
>> + __leave;
>> + }
>> if (!*oldpath || !*newpath)
>> {
>> /* Reject rename("","x"), rename("x",""). */
>> @@ -2337,6 +2348,13 @@ rename (const char *oldpath, const char *newpath)
>> __leave;
>> }
>>
>> + /* Should we replace an existing file? */
>> + if ((removepc || dstpc->exists ()) && (flags & RENAME_NOREPLACE))
>> + {
>> + set_errno (EEXIST);
>> + __leave;
>> + }
>> +
>
> Do we really need this test here? If you check at this point and then
> go ahead preparing the actual rename operation, you have the atomicity
> problem again which renameat2 is trying to solve.
>
> But there's light. NtSetInformationFile(FileRenameInformation) already
> supports RENAME_NOREPLACE :)
>
> Have a look at line 2494 (prior to your patch):
>
> pfri->ReplaceIfExists = TRUE;
>
> if you replace this with something like
>
> pfri->ReplaceIfExists = !(flags & RENAME_NOREPLACE);
>
> it should give us the atomic behaviour of renameat2 on Linux.
>
> Another upside is, the status code returned is STATUS_OBJECT_NAME_COLLISION,
> which translates to Win32 error ERROR_ALREADY_EXISTS, which in turn is
> already converted to EEXIST by Cygwin, so there's nothing more to do :)
>
> What do you think?
Thanks for the improvements! A revised patch is attached. As you'll
see, I still found a few places where I thought I needed to explicitly
set the errno to EEXIST. Let me know if any of these could be avoided.
Thanks.
Ken
-------------- next part --------------
From d5798b371ceabfe6a7912472edd32da1ebd7dcb7 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Thu, 17 Aug 2017 09:12:15 -0400
Subject: [PATCH] cygwin: Implement renameat2
Define the RENAME_NOREPLACE flag in <cygwin/fs.h> as defined on Linux
in <linux/fs.h>. The other RENAME_* flags defined on Linux are not
supported.
---
newlib/libc/include/stdio.h | 3 ++
winsup/cygwin/common.din | 1 +
winsup/cygwin/include/cygwin/fs.h | 6 +++
winsup/cygwin/include/cygwin/version.h | 3 +-
winsup/cygwin/syscalls.cc | 67 +++++++++++++++++++++++++++++++---
5 files changed, 73 insertions(+), 7 deletions(-)
diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h
index 5d8cb1092..331a1cf07 100644
--- a/newlib/libc/include/stdio.h
+++ b/newlib/libc/include/stdio.h
@@ -384,6 +384,9 @@ int _EXFUN(vdprintf, (int, const char *__restrict, __VALIST)
#endif
#if __ATFILE_VISIBLE
int _EXFUN(renameat, (int, const char *, int, const char *));
+# ifdef __CYGWIN__
+int _EXFUN(renameat2, (int, const char *, int, const char *, unsigned int));
+# endif
#endif
/*
diff --git a/winsup/cygwin/common.din b/winsup/cygwin/common.din
index 8da432b8a..ca6ff3cf9 100644
--- a/winsup/cygwin/common.din
+++ b/winsup/cygwin/common.din
@@ -1168,6 +1168,7 @@ remquof NOSIGFE
remquol NOSIGFE
rename SIGFE
renameat SIGFE
+renameat2 SIGFE
res_close = __res_close SIGFE
res_init = __res_init SIGFE
res_mkquery = __res_mkquery SIGFE
diff --git a/winsup/cygwin/include/cygwin/fs.h b/winsup/cygwin/include/cygwin/fs.h
index f606ffc39..48b0cca45 100644
--- a/winsup/cygwin/include/cygwin/fs.h
+++ b/winsup/cygwin/include/cygwin/fs.h
@@ -19,4 +19,10 @@ details. */
#define BLKPBSZGET 0x0000127b
#define BLKGETSIZE64 0x00041268
+/* Flags for renameat2, from /usr/include/linux/fs.h. For now we
+ support only RENAME_NOREPLACE. */
+#define RENAME_NOREPLACE (1 << 0)
+/* #define RENAME_EXCHANGE (1 << 1) */
+/* #define RENAME_WHITEOUT (1 << 2) */
+
#endif
diff --git a/winsup/cygwin/include/cygwin/version.h b/winsup/cygwin/include/cygwin/version.h
index efd4ac017..7686a6865 100644
--- a/winsup/cygwin/include/cygwin/version.h
+++ b/winsup/cygwin/include/cygwin/version.h
@@ -481,12 +481,13 @@ details. */
314: Export explicit_bzero.
315: Export pthread_mutex_timedlock.
316: Export pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock.
+ 317: Export renameat2.
Note that we forgot to bump the api for ualarm, strtoll, strtoull,
sigaltstack, sethostname. */
#define CYGWIN_VERSION_API_MAJOR 0
-#define CYGWIN_VERSION_API_MINOR 316
+#define CYGWIN_VERSION_API_MINOR 317
/* There is also a compatibity version number associated with the shared memory
regions. It is incremented when incompatible changes are made to the shared
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 885931632..2758bb776 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -60,6 +60,7 @@ details. */
#include "tls_pbuf.h"
#include "sync.h"
#include "child_info.h"
+#include <cygwin/fs.h> /* needed for RENAME_NOREPLACE */
#undef _close
#undef _lseek
@@ -2048,14 +2049,19 @@ nt_path_has_executable_suffix (PUNICODE_STRING upath)
return false;
}
-extern "C" int
-rename (const char *oldpath, const char *newpath)
+/* If newpath names an existing file and the RENAME_NOREPLACE flag is
+ specified, fail with EEXIST. Exception: Don't fail if the purpose
+ of the rename is just to change the case of oldpath on a
+ case-insensitive file system. */
+static int
+rename2 (const char *oldpath, const char *newpath, unsigned int flags)
{
tmp_pathbuf tp;
int res = -1;
path_conv oldpc, newpc, new2pc, *dstpc, *removepc = NULL;
bool old_dir_requested = false, new_dir_requested = false;
bool old_explicit_suffix = false, new_explicit_suffix = false;
+ bool noreplace = flags & RENAME_NOREPLACE;
size_t olen, nlen;
bool equal_path;
NTSTATUS status = STATUS_SUCCESS;
@@ -2068,6 +2074,12 @@ rename (const char *oldpath, const char *newpath)
__try
{
+ if (flags & ~RENAME_NOREPLACE)
+ /* RENAME_NOREPLACE is the only flag currently supported. */
+ {
+ set_errno (EINVAL);
+ __leave;
+ }
if (!*oldpath || !*newpath)
{
/* Reject rename("","x"), rename("x",""). */
@@ -2337,6 +2349,17 @@ rename (const char *oldpath, const char *newpath)
__leave;
}
+ /* If a removepc exists and RENAME_NOREPLACE has been specified,
+ then we want to fail with EEXIST. But dstpc points to a
+ non-existing file, so the setting of ReplaceIfExists below
+ will not cause the failure. We must therefore do it
+ explicitly. */
+ if (removepc && noreplace)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
+
/* Opening the file must be part of the transaction. It's not sufficient
to call only NtSetInformationFile under the transaction. Therefore we
have to start the transaction here, if necessary. */
@@ -2410,6 +2433,11 @@ rename (const char *oldpath, const char *newpath)
unlink_nt returns with STATUS_DIRECTORY_NOT_EMPTY. */
if (dstpc->isdir ())
{
+ if (noreplace)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
status = unlink_nt (*dstpc);
if (!NT_SUCCESS (status))
{
@@ -2423,6 +2451,11 @@ rename (const char *oldpath, const char *newpath)
due to a mangled suffix. */
else if (!removepc && dstpc->has_attribute (FILE_ATTRIBUTE_READONLY))
{
+ if (noreplace)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
status = NtOpenFile (&nfh, FILE_WRITE_ATTRIBUTES,
dstpc->get_object_attr (attr, sec_none_nih),
&io, FILE_SHARE_VALID_FLAGS,
@@ -2491,11 +2524,15 @@ rename (const char *oldpath, const char *newpath)
__leave;
}
pfri = (PFILE_RENAME_INFORMATION) tp.w_get ();
- pfri->ReplaceIfExists = TRUE;
+ pfri->ReplaceIfExists = !noreplace;
pfri->RootDirectory = NULL;
pfri->FileNameLength = dstpc->get_nt_native_path ()->Length;
memcpy (&pfri->FileName, dstpc->get_nt_native_path ()->Buffer,
pfri->FileNameLength);
+ /* If dstpc points to an existing file and RENAME_NOREPLACE has
+ been specified, then we should get NT error
+ STATUS_OBJECT_NAME_COLLISION ==> Win32 error
+ ERROR_ALREADY_EXISTS ==> Cygwin error EEXIST. */
status = NtSetInformationFile (fh, &io, pfri,
sizeof *pfri + pfri->FileNameLength,
FileRenameInformation);
@@ -2509,6 +2546,11 @@ rename (const char *oldpath, const char *newpath)
if (status == STATUS_ACCESS_DENIED && dstpc->exists ()
&& !dstpc->isdir ())
{
+ if (noremove)
+ {
+ set_errno (EEXIST);
+ __leave;
+ }
bool need_open = false;
if ((dstpc->fs_flags () & FILE_SUPPORTS_TRANSACTIONS) && !trans)
@@ -2578,6 +2620,12 @@ rename (const char *oldpath, const char *newpath)
return res;
}
+extern "C" int
+rename (const char *oldpath, const char *newpath)
+{
+ return rename2 (oldpath, newpath, 0);
+}
+
extern "C" int
system (const char *cmdstring)
{
@@ -4719,8 +4767,8 @@ readlinkat (int dirfd, const char *__restrict pathname, char *__restrict buf,
}
extern "C" int
-renameat (int olddirfd, const char *oldpathname,
- int newdirfd, const char *newpathname)
+renameat2 (int olddirfd, const char *oldpathname,
+ int newdirfd, const char *newpathname, unsigned int flags)
{
tmp_pathbuf tp;
__try
@@ -4731,13 +4779,20 @@ renameat (int olddirfd, const char *oldpathname,
char *newpath = tp.c_get ();
if (gen_full_path_at (newpath, newdirfd, newpathname))
__leave;
- return rename (oldpath, newpath);
+ return rename2 (oldpath, newpath, flags);
}
__except (EFAULT) {}
__endtry
return -1;
}
+extern "C" int
+renameat (int olddirfd, const char *oldpathname,
+ int newdirfd, const char *newpathname)
+{
+ return renameat2 (olddirfd, oldpathname, newdirfd, newpathname, 0);
+}
+
extern "C" int
scandirat (int dirfd, const char *pathname, struct dirent ***namelist,
int (*select) (const struct dirent *),
--
2.14.1
More information about the Cygwin-patches
mailing list