Re: renameat2

[Corinna Vinschen]

On Aug 19 13:24, Ken Brown wrote:
> On 8/19/2017 12:28 PM, Corinna Vinschen wrote:
> > Doc changes coming? :)
> 
> Attached.

Pushed.  I also generated new developer snapshots.


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


signature.asc
Description: PGP signature

Re: renameat2

[Corinna Vinschen]

On Aug 19 13:13, Ken Brown wrote:
> On 8/19/2017 12:37 PM, Corinna Vinschen wrote:
> > Oh, one more thing.  This is a question to Yaakov, too.
> > 
> > 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
> > 
> > Does it makes sense to guard the renameat2 prototype more extensively
> > to cater for standards junkies?  __MISC_VISIBLE, perhaps?
> 
> I'll defer to Yaakov.  But here's a related question.  Is renameat
> currently guarded properly?  The Linux man page says:
> 
> Feature Test Macro Requirements for glibc (see feature_test_macros(7)):
> 
>renameat():
>Since glibc 2.10:
>_POSIX_C_SOURCE >= 200809L
>Before glibc 2.10:
>_ATFILE_SOURCE
> 
> This suggests that we should do something like the following:
> 
> diff --git a/newlib/libc/include/stdio.h b/newlib/libc/include/stdio.h
> index 331a1cf07..9eb0964f2 100644
> --- a/newlib/libc/include/stdio.h
> +++ b/newlib/libc/include/stdio.h
> @@ -381,8 +381,6 @@ FILE *  _EXFUN(open_memstream, (char **, size_t *));
>  int_EXFUN(vdprintf, (int, const char *__restrict, __VALIST)
> _ATTRIBUTE ((__format__ (__printf__, 2, 0;
>  # endif
> -#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));

No, that's correct.  See sys/features.h:

 * __ATFILE_VISIBLE
 *  "at" functions; enabled by default, with _ATFILE_SOURCE,
 *  _POSIX_C_SOURCE >= 200809L, or _XOPEN_SOURCE >= 700.


Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


signature.asc
Description: PGP signature

Re: renameat2

[Ken Brown]

On 8/19/2017 12:28 PM, Corinna Vinschen wrote:

Doc changes coming? :)


Attached.

Ken

From 0704541f1d29e0d9aa0af6e549f8ca0114a44a7c Mon Sep 17 00:00:00 2001
From: Ken Brown 
Date: Sat, 19 Aug 2017 13:15:04 -0400
Subject: [PATCH] Document renameat2

---
 winsup/cygwin/release/2.9.0 | 2 ++
 winsup/doc/new-features.xml | 4 
 winsup/doc/posix.xml| 4 
 3 files changed, 10 insertions(+)

diff --git a/winsup/cygwin/release/2.9.0 b/winsup/cygwin/release/2.9.0
index 421d6f24f..ac4c64949 100644
--- a/winsup/cygwin/release/2.9.0
+++ b/winsup/cygwin/release/2.9.0
@@ -6,6 +6,8 @@ What's new:
 - New APIs: pthread_mutex_timedwait, pthread_rwlock_timedrdlock,
pthread_rwlock_timedwrlock.
 
+- New API: renameat2.
+
 
 What changed:
 -
diff --git a/winsup/doc/new-features.xml b/winsup/doc/new-features.xml
index 23673d1e0..0aa857730 100644
--- a/winsup/doc/new-features.xml
+++ b/winsup/doc/new-features.xml
@@ -17,6 +17,10 @@ New APIs: pthread_mutex_timedwait, 
pthread_rwlock_timedrdlock,
 pthread_rwlock_timedwrlock.
 
 
+
+New API: renameat2.
+
+
 
 Improved implementation of elf.h.
 
diff --git a/winsup/doc/posix.xml b/winsup/doc/posix.xml
index a2fffeebf..6e96272b7 100644
--- a/winsup/doc/posix.xml
+++ b/winsup/doc/posix.xml
@@ -1356,6 +1356,7 @@ also IEEE Std 1003.1-2008 (POSIX.1-2008).
 ptsname_r
 putwc_unlocked
 putwchar_unlocked
+renameat2  (see chapter "Implementation Notes")
 qsort_r(see chapter "Implementation Notes")
 quotactl
 rawmemchr
@@ -1671,6 +1672,9 @@ group quotas, no inode quotas, no time constraints.
 qsort_r is available in both BSD and GNU flavors,
 depending on whether _BSD_SOURCE or _GNU_SOURCE is defined when 
compiling.
 
+The Linux-specific function renameat2 only
+supports the RENAME_NOREPLACE flag.
+
 basename is available in both POSIX and GNU flavors,
 depending on whether libgen.h is included or not.
 
-- 
2.14.1

Re: renameat2

[Corinna Vinschen]

On Aug 19 18:28, Corinna Vinschen wrote:
> On Aug 19 10:29, Ken Brown wrote:
> > Hi Corinna,
> > 
> > On 8/19/2017 5:57 AM, Corinna Vinschen wrote:
> > > Hi Ken,
> > > 
> > > On Aug 18 18:24, Ken Brown wrote:
> > > The patch is ok as is, just let me know what you think of the above
> > > minor tweak (and send the revised patch if you agree).
> > 
> > Yes, I agree.  But can't I also drop the third test (where you said "good
> > catch") for the same reason?  I've done that in the attached.  If I'm wrong
> > and I still need that third test, let me know and I'll put it back.
> 
> Nope, you're right.  Same rules apply for the third test.  Patch pushed.
> Doc changes coming? :)

Oh, one more thing.  This is a question to Yaakov, too.

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

Does it makes sense to guard the renameat2 prototype more extensively
to cater for standards junkies?  __MISC_VISIBLE, perhaps?


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


signature.asc
Description: PGP signature

Re: renameat2

[Corinna Vinschen]

On Aug 19 10:29, Ken Brown wrote:
> Hi Corinna,
> 
> On 8/19/2017 5:57 AM, Corinna Vinschen wrote:
> > Hi Ken,
> > 
> > On Aug 18 18:24, Ken Brown wrote:
> > The patch is ok as is, just let me know what you think of the above
> > minor tweak (and send the revised patch if you agree).
> 
> Yes, I agree.  But can't I also drop the third test (where you said "good
> catch") for the same reason?  I've done that in the attached.  If I'm wrong
> and I still need that third test, let me know and I'll put it back.

Nope, you're right.  Same rules apply for the third test.  Patch pushed.
Doc changes coming? :)


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


signature.asc
Description: PGP signature

Re: renameat2

[Corinna Vinschen]

Hi Ken,

On Aug 18 18:24, Ken Brown wrote:
> Hi Corinna,
> 
> On 8/18/2017 11:15 AM, Corinna Vinschen wrote:
> > On Aug 18 09:21, Ken Brown wrote:
> > 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.

No, you're right.  Just one thing:

> @@ -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 (, FILE_WRITE_ATTRIBUTES,
>  dstpc->get_object_attr (attr, sec_none_nih),
>  , FILE_SHARE_VALID_FLAGS,

Both of the above cases are just border cases of one and the same
problem, the destination file already exists.

In retrospect your original patch was more concise:

+  /* Should we replace an existing file? */
+  if ((removepc || dstpc->exists ()) && (flags & RENAME_NOREPLACE))
+   {
+ set_errno (EEXIST);
+ __leave;
+   }

The atomicity considerations are not affected by this test anyway, but
it would avoid starting and stopping a transaction on NTFS for no good
reason.

Maybe it's better to revert to this test and drop the other two again?

> @@ -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 (>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, , 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;
> + }

Oh, right, that's a good catch.

The patch is ok as is, just let me know what you think of the above
minor tweak (and send the revised patch if you agree).


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


signature.asc
Description: PGP signature

Re: renameat2

[Ken Brown]

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   0x127b
  #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 
Date: Thu, 17 Aug 2017 09:12:15 -0400
Subject: [PATCH] cygwin: Implement renameat2

Define the RENAME_NOREPLACE flag in  as defined on Linux
in .  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 

Re: renameat2

[Ken Brown]

On 8/18/2017 10:33 AM, Eric Blake wrote:

Please only define RENAME_NOREPLACE for now; that way, software can
probe what is defined to know what will work (defining a flag that will
always be an error is not as useful as leaving it undefined - and while
we may add RENAME_EXCHANGE support, I don't see how we can ever do
RENAME_WHITEOUT).


Thanks for the feedback.  Revised patch attached.

From 136b0dfd53e147002e134048658d20f452402c9f Mon Sep 17 00:00:00 2001
From: Ken Brown 
Date: Thu, 17 Aug 2017 09:12:15 -0400
Subject: [PATCH] cygwin: Implement renameat2

Define the RENAME_NOREPLACE flag in  as defined on Linux
in .  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  | 41 +-
 5 files changed, 48 insertions(+), 6 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   0x127b
 #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..0384f77da 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.  Add RENAME_NOREPLACE.
 
   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..d756f5f35 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   /* needed for RENAME_NOREPLACE */
 
 #undef _close
 #undef _lseek
@@ -2048,8 +2049,12 @@ 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;
@@ -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);
+ __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;
+   }
+
   /* 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. */
@@ -2578,6