On 26-01-2021 12:34, Corinna Vinschen via Cygwin-patches wrote:
>
> First of all, the new function should better get a new name. The _nt
> postfix is pretty much historical anyway to differentiate between the
> function using Win32 API and the function using NT API. This is kind
> of moot these days sine we're using the NT API almost exclusively for
> file access anyway.
>
> So stick to unlink_nt/_unlink_nt for the existing functions, and name
> the new function accorind to it's doings, like, say, unlink_path or
> whatever.
>
Sure, I will rename them.
>> + static bool has_posix_unlink_semantics =
>> + wincap.has_posix_unlink_semantics ();
>> + static bool has_posix_unlink_semantics_with_ignore_readonly =
>> + wincap.has_posix_unlink_semantics_with_ignore_readonly ();
>
> Did you mean `const' rather than `static', by any chance? Either way, I
> don't think these local vars are required, given that the wincap
> accessors are already marked as const. The compiler should know how to
> opimize this sufficiently.
>
I do mean static.
With each instantiation these are initialized to the wincap value.
Then later on, we might disable the behavior if we encounter a driver
which returns: STATUS_INVALID_PARAMETER
Assuming most files will reside on the same fs, (ie through the same driver)
this will save use from the system call which we know isn't supported.
>> +
>> + HANDLE fh;
>> + ACCESS_MASK access = DELETE;
>> + IO_STATUS_BLOCK io;
>> + ULONG flags = FILE_OPEN_REPARSE_POINT | FILE_OPEN_FOR_BACKUP_INTENT
>> + | FILE_DELETE_ON_CLOSE | eflags;
>
> This looks like a dangerous assumption. So far we don't open unknown
> reparse points as reparse points deliberately. No one knows what a
> unknown reparse point is good for or supposed to do, so we don't even
> know if we are allowed to handle it analogue to a symlink.
>
When opening these, you are correct.
However, when a request is made to delete a reparse point, it's safe
- even for an unknown reparse point - to assume that it is the reparse point
itself which is to be deleted. Ofcourse: That's my theory.
> Consequentially we open unknown reparse points just as normal files, so
> that the reparse point's automatisms may kick in. By omitting this
> step, we're moving on thin ice.
>
This would mean an unknown reparse point can never be deleted.
I'm just not sure if that's what we should want.
>> +{
>> + //Step 2
>> + //Reopen with all sharing flags, will set delete flag ourselves.
>> + access |= FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES;
>> + flags &= ~FILE_DELETE_ON_CLOSE;
>> + fstatus = NtOpenFile (, access, attr, , FILE_SHARE_VALID_FLAGS,
>> flags);
>> + debug_printf ("NtOpenFile %S: %y", attr->ObjectName, fstatus);
>> +
>> + if (NT_SUCCESS (fstatus))
>> +{
>> + if (has_posix_unlink_semantics_with_ignore_readonly)
>> +{
>> + //Step 3
>> + //Remove the file with POSIX unlink semantics, ignore
>> readonly flags.
>
> No check for NTFS? Posix semantics are not supported on any other FS.
> No check for remote? Just because you support POSIX semantics on
> *this* machine, doesn't mean the remote machine supports it at all...
>
Indeed no checks.
If the driver correctly returns STATUS_INVALID_PARAMETER we will not try again
(by
resetting the has_posix_unlink_semantics_with_ignore_readonly flag and then
fallback to
usual trickery. If the driver returns error (but not STATUS_INVALID_PARAMETER)
that
driver pays a single kernel call, which I deem acceptable.
>> + FILE_DISPOSITION_INFORMATION_EX fdie =
>> +{ FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS
>> +| FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE };
>> + istatus = NtSetInformationFile (fh, , , sizeof fdie,
>> + FileDispositionInformationEx);
>> + debug_printf ("NtSetInformation %S: %y", attr->ObjectName,
>> istatus);
>> +
>> + if(istatus == STATUS_INVALID_PARAMETER)
>> +has_posix_unlink_semantics_with_ignore_readonly = false;
>> +}
>> +
>> + if (!has_posix_unlink_semantics_with_ignore_readonly
>> + || !NT_SUCCESS (istatus))
>> +{
>> + //Step 4
>> + //Check if we must clear the READONLY flag
>> + FILE_BASIC_INFORMATION qfbi = { 0 };
>> + istatus = NtQueryInformationFile (fh, , , sizeof qfbi,
>> +FileBasicInformation);
>> + debug_printf ("NtQueryFileBasicInformation %S: %y",
>> +attr->ObjectName, istatus);
>> +
>> + if (NT_SUCCESS (istatus))
>> +{
>> + if (qfbi.FileAttributes & FILE_ATTRIBUTE_READONLY)
>> +{
>> + //Step 5
>> +
