Re: renameat2

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

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,
Corinna

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

Attachment: signature.asc
Description: PGP signature