renameat2
Corinna Vinschen
corinna-cygwin@cygwin.com
Sat Aug 19 16:37:00 GMT 2017
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://cygwin.com/pipermail/cygwin-patches/attachments/20170819/2dba9caa/attachment.sig>
More information about the Cygwin-patches
mailing list