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] |
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, Istill 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] |