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

I have seen bordercases, but I have not seen NtSetInformation fail
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