This is the mail archive of the cygwin-patches mailing list for the Cygwin 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] |
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
Attachment:
0001-cygwin-Implement-renameat2.patch
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |