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]

Re: renameat2


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]