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
From d5798b371ceabfe6a7912472edd32da1ebd7dcb7 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbr...@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
