[PATCH 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first

Ben ben@wijen.net
Mon Jan 18 14:30:24 GMT 2021



On 18-01-2021 13:22, Corinna Vinschen via Cygwin-patches wrote:
> On Jan 18 13:11, Ben wrote:
>>
>>
>> On 18-01-2021 11:45, Corinna Vinschen via Cygwin-patches wrote:
>>> Rather than calling NtSetInformationFile here again, we should rather
>>> just skip the transaction stuff on 1809 and later.  I'd suggest adding
>>> another wincap flag like, say, "has_posix_ro_override", being true
>>> for 1809 and later.  Then we can skip the transaction handling if
>>> wincap.has_posix_ro_override () and just add the
>>> FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE flag to fdie.Flags, if
>>> it's available.
>>
>> Hmmm, I'm not sure if I follow you: This extra NtSetInformationFile is not
>> related to the transaction stuff?
> 
> Right, sorry.  I meant the
> 
>   if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
> 
> bracketed code in fact.  What I meant is to keep it at
> 
>   open
>   if (ro)
>     setattributes
>   setinformation
>   ...
> 
> and only add the required additional flag.

Ah, yes I understand. The extra NtSetInformation was there for
the fallback without FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

I have seen bordercases, but I have not seen NtSetInformation fail
FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE and succeed without.
Even if it would, Your suggestion does save a bunch of code...

> 
> 
>> Also I have seen NtSetInformationFile fail with STATUS_INVALID_PARAMETER.
> 
> That should only occur on pre-1809 then, and adding the extra wincap
> would fix that.

Do note: This can also happen post-1809 with a driver that hasn't implemented it yet.

> 
>> So a retry without FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE is valid here.
> 
> That would be a border case which might then occur with the
> FILE_DISPOSITION_POSIX_SEMANTICS flag, too.  The current code falls
> through anyway, that should be sufficient, right?

Yes, the existing fallback, should be sufficient.

> 
>>
>> I have thought about adding wincap.has_posix_unlink_semantics_with_ignore_readonly
>> but it is equal to wincap.has_posix_rename_semantics so I didn't bother adding it.
> 
> It doesn't hurt to add another flag with the same values.  It's better
> readable in context, which easily makes up for the extra bit :)

Ok, will do.

> 
> 
> Corinna
> 


More information about the Cygwin-patches mailing list