Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt
On Feb 3 12:03, Ben wrote:
> On 26-01-2021 12:34, Corinna Vinschen via Cygwin-patches wrote:
> >> + 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.
But this is an invalid assumption. In fact, it only hold true for
`rm -r' scenarios, not for anything else. What about long running
processes unlinking files in various spots under /var, /tmp, etc.,
i. .e. services. Or what about mc?
Keep in mind that unlink/rmdir are system calls. Each an every call is
independent of each other. If you encounter two unlink calls, you don't
even know how much time has gone by between them. Even if they occur in
a short time frame, you still don't know if they are even remotely
related. For all you know, they could be called from different threads,
doing different things on different partitions.
As such, using a static state at this point and keeping it around for
later calls is a bug.
> >> + 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.
It's definitely a deviation from the previous behaviour and I'm not
exactly comfortable with it. The problem is that only a minor part of
the reparse point population is actually something akin to a symlink. I
don't see how it can be a safe bet to allow the user to remove an RP
with unknown and, perhaps, crucial functionality to some given product.
> > 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.
It's what we do right now. We're trying to handle all RP types known to
constitute some kind of symlink, and if we learn about the meaning of as
yet unknown RPs, and it *is* some kind of symlink, we can add it to the
list.
> >> +{
> >> + //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.
That's back to the static var then, which isn't feasible. For non-NTFS
or remote FSes you will introduce a constant penalty.
> >> + //As we don't have posix unlink semantics, this
> >> will still fail if the file is in use.
> >
> > Without transaction?
> >
> Well, yes, the transaction overhead doesn't weigh up to the unlikeliness of
> failure, I think.
The transaction would only be called for DOS R/O files anyway, which is
a minor part of the file population with a pretty
Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt
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
>> +
Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt
Hi Ben,
ok, this is strong stuff, and apart from a couple of formatting issues,
we should really discuss if a couple of things are feasible at all.
On Jan 20 17:10, Ben Wijen wrote:
> Implement _unlink_nt: wich does not depend on patch_conv
> ---
> winsup/cygwin/fhandler_disk_file.cc | 4 +-
> winsup/cygwin/forkable.cc | 4 +-
> winsup/cygwin/syscalls.cc | 211 ++--
> 3 files changed, 200 insertions(+), 19 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler_disk_file.cc
> b/winsup/cygwin/fhandler_disk_file.cc
> index 07f9c513a..fe04f832b 100644
> --- a/winsup/cygwin/fhandler_disk_file.cc
> +++ b/winsup/cygwin/fhandler_disk_file.cc
> @@ -1837,7 +1837,7 @@ fhandler_disk_file::mkdir (mode_t mode)
> int
> fhandler_disk_file::rmdir ()
> {
> - extern NTSTATUS unlink_nt (path_conv );
> + extern NTSTATUS unlink_ntpc (path_conv );
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.
> @@ -695,7 +695,157 @@ _unlink_nt_post_dir_check (NTSTATUS status,
> POBJECT_ATTRIBUTES attr, const path_
> }
>
> static NTSTATUS
> -_unlink_nt (path_conv , bool shareable)
> +_unlink_nt (POBJECT_ATTRIBUTES attr, ULONG eflags)
> +{
> + 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.
> +
> + 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.
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.
> + NTSTATUS fstatus, istatus = STATUS_SUCCESS;
> +
> + syscall_printf("Trying to delete %S, isdir = %d", attr->ObjectName,
> + eflags == FILE_DIRECTORY_FILE);
> +
> + //FILE_DELETE_ON_CLOSE icw FILE_DIRECTORY_FILE only works when directory
> is empty
> + //-> We must assume directory not empty, therefore only do this for
> regular files.
Please use C-style /* ... */ comments in the first place, especially
on multi-line comments.
Also, please keep the line length below 80 chars where possible.
> + if (eflags & FILE_NON_DIRECTORY_FILE)
> +{
> + //Step 1
> + //If the file is not 'in use' and not 'readonly', this should just
> work.
> + fstatus = NtOpenFile (, access, attr, , FILE_SHARE_DELETE,
> flags);
> + debug_printf ("NtOpenFile %S: %y", attr->ObjectName, fstatus);
> +}
> +
> + if (!(eflags & FILE_NON_DIRECTORY_FILE)// Workaround for the
> non-empty-dir issue
> + || fstatus == STATUS_SHARING_VIOLATION // The file is 'in use'
> + || fstatus == STATUS_CANNOT_DELETE)// The file is 'readonly'
I'd drop these comments, the status codes are somewhat self-explaining.
> +{
> + //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...
> + FILE_DISPOSITION_INFORMATION_EX fdie =
> +{ FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS
> +| FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE };
> + istatus = NtSetInformationFile (fh, , , sizeof fdie,
> +
[PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt
Implement _unlink_nt: wich does not depend on patch_conv
---
winsup/cygwin/fhandler_disk_file.cc | 4 +-
winsup/cygwin/forkable.cc | 4 +-
winsup/cygwin/syscalls.cc | 211 ++--
3 files changed, 200 insertions(+), 19 deletions(-)
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index 07f9c513a..fe04f832b 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1837,7 +1837,7 @@ fhandler_disk_file::mkdir (mode_t mode)
int
fhandler_disk_file::rmdir ()
{
- extern NTSTATUS unlink_nt (path_conv );
+ extern NTSTATUS unlink_ntpc (path_conv );
if (!pc.isdir ())
{
@@ -1850,7 +1850,7 @@ fhandler_disk_file::rmdir ()
return -1;
}
- NTSTATUS status = unlink_nt (pc);
+ NTSTATUS status = unlink_ntpc (pc);
if (!NT_SUCCESS (status))
{
diff --git a/winsup/cygwin/forkable.cc b/winsup/cygwin/forkable.cc
index 350a95c3e..bd7421bf5 100644
--- a/winsup/cygwin/forkable.cc
+++ b/winsup/cygwin/forkable.cc
@@ -29,7 +29,7 @@ details. */
/* Allow concurrent processes to use the same dll or exe
* via their hardlink while we delete our hardlink. */
-extern NTSTATUS unlink_nt_shareable (path_conv );
+extern NTSTATUS unlink_ntpc_shareable (path_conv );
#define MUTEXSEP L"@"
#define PATHSEP L"\\"
@@ -132,7 +132,7 @@ rmdirs (WCHAR ntmaxpathbuf[NT_MAX_PATH])
RtlInitUnicodeString (, ntmaxpathbuf);
path_conv pc ();
- unlink_nt_shareable (pc); /* move to bin */
+ unlink_ntpc_shareable (pc); /* move to bin */
}
if (!pfdi->NextEntryOffset)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index b220bedae..ab0c4c2d6 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -491,7 +491,7 @@ try_to_bin (path_conv , HANDLE , ACCESS_MASK access,
ULONG flags)
break;
case STATUS_DIRECTORY_NOT_EMPTY:
/* Uh oh! This was supposed to be avoided by the check_dir_not_empty
-test in unlink_nt, but given that the test isn't atomic, this *can*
+test in unlink_ntpc, but given that the test isn't atomic, this *can*
happen. Try to move the dir back ASAP. */
pfri->RootDirectory = NULL;
pfri->FileNameLength = pc.get_nt_native_path ()->Length;
@@ -501,7 +501,7 @@ try_to_bin (path_conv , HANDLE , ACCESS_MASK access,
ULONG flags)
if (NT_SUCCESS (NtSetInformationFile (fh, , pfri, frisiz,
FileRenameInformation)))
{
- /* Give notice to unlink_nt and leave immediately. This avoids
+ /* Give notice to unlink_ntpc and leave immediately. This avoids
closing the handle, which might still be used if called from
the rm -r workaround code. */
bin_stat = dir_not_empty;
@@ -545,7 +545,7 @@ try_to_bin (path_conv , HANDLE , ACCESS_MASK access,
ULONG flags)
if ((access & FILE_WRITE_ATTRIBUTES) && NT_SUCCESS (status) && !pc.isdir ())
NtSetAttributesFile (fh, pc.file_attributes ());
NtClose (fh);
- fh = NULL; /* So unlink_nt doesn't close the handle twice. */
+ fh = NULL; /* So unlink_ntpc doesn't close the handle twice. */
/* On success or when trying to unlink a directory we just return here.
The below code only works for files. It also fails on NFS. */
if (NT_SUCCESS (status) || pc.isdir () || pc.fs_is_nfs ())
@@ -695,7 +695,157 @@ _unlink_nt_post_dir_check (NTSTATUS status,
POBJECT_ATTRIBUTES attr, const path_
}
static NTSTATUS
-_unlink_nt (path_conv , bool shareable)
+_unlink_nt (POBJECT_ATTRIBUTES attr, ULONG eflags)
+{
+ 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 ();
+
+ 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;
+ NTSTATUS fstatus, istatus = STATUS_SUCCESS;
+
+ syscall_printf("Trying to delete %S, isdir = %d", attr->ObjectName,
+ eflags == FILE_DIRECTORY_FILE);
+
+ //FILE_DELETE_ON_CLOSE icw FILE_DIRECTORY_FILE only works when directory is
empty
+ //-> We must assume directory not empty, therefore only do this for regular
files.
+ if (eflags & FILE_NON_DIRECTORY_FILE)
+{
+ //Step 1
+ //If the file is not 'in use' and not 'readonly', this should just work.
+ fstatus = NtOpenFile (, access, attr, , FILE_SHARE_DELETE, flags);
+ debug_printf ("NtOpenFile %S: %y", attr->ObjectName, fstatus);
+}
+
+ if (!(eflags & FILE_NON_DIRECTORY_FILE)// Workaround for the
non-empty-dir issue
+ || fstatus == STATUS_SHARING_VIOLATION // The file is 'in use'
+ || fstatus == STATUS_CANNOT_DELETE)// The file is 'readonly'
+{
+ //Step 2
+
