Re: rename2 check for existing file

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

On Jan  9 23:09, Ken Brown wrote:
> On 1/9/2019 3:37 PM, Ken Brown wrote:
> > Hi Corinna,
> > 
> > On 1/9/2019 9:52 AM, Corinna Vinschen wrote:
> >> Hi Ken,
> >>
> >> your patch to support renameat2(..., RENAME_NOREPLACE),
> >> commit f665b1cef30f9032877081ac63ea94910825be6a, also
> >> introduced a new check
> >>
> >> +      /* Should we replace an existing file? */
> >> +      if ((removepc || dstpc->exists ()) && noreplace)
> >> +       {
> >> +         set_errno (EEXIST);
> >> +         __leave;
> >> +       }
> >>
> >> However, the noreplace flag also adds the same check to the actual
> >> NtSetInformationFile call to rename the file:
> >>
> >> -      pfri->ReplaceIfExists = TRUE;
> >> +      pfri->ReplaceIfExists = !noreplace;
> >>
> >> So, isn't the first check redundant?  Can't we just drop it?  The
> >> rename2 function already has so many checks to perform before actually
> >> calling NtSetInformationFile, every check we can remove is a boon, I
> >> think.
> > 
> > My recollection is that that we discussed this at the time and decided that
> > there was a border case where the check was still needed in order to make sure
> > that EEXIST was set.  I'll have to look back at the email thread and see if I
> > can reconstruct the reason.
> 
> I think there were three cases in which the check was needed:
> 
>    - removepc is non-NULL
>    - dstpc points to an existing directory
>    - dstpc points to an existing file with FILE_ATTRIBUTE_READONLY.
> 
> But now that you've introduced use_posix_semantics, maybe the check is only 
> needed in the pre-W10 case.

Thanks for looking.  I'm not entirely sure yet if we can drop this with
POSIX semantics in terms of removepc != NULL.  The other two cases are
covered by the new POSIX rename, afaics.  I'll check that when I get the
time.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

Attachment: signature.asc
Description: PGP signature