rename2 check for existing file

rename2 check for existing file

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

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.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: rename2 check for existing file

[Ken Brown]

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 may not have a chance to do that until tomorrow.

Ken

Re: rename2 check for existing file

[Ken Brown]

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.

Ken

Re: rename2 check for existing file

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]