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 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 (&nfh, FILE_WRITE_ATTRIBUTES,
>  			       dstpc->get_object_attr (attr, sec_none_nih),
>  			       &io, 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 (&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;
> +	    }

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

Attachment: signature.asc
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]