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