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/19/2017 5:57 AM, Corinna Vinschen wrote:
Hi Ken,

On Aug 18 18:24, Ken Brown wrote:
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).

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.

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]