Re: [PATCH 09/11] mount.cc: Implement poor-man's cache
On 18-01-2021 12:51, Corinna Vinschen via Cygwin-patches wrote: > Ok, so hash_prefix reduces the path to a drive letter or the UNC path > prefix and hashes it. However, what about partitions mounted to a > subdir of, say, drive C? In that case the hashing goes awry, because > you're comparing with the hash of drive C while the path is actually > pointing to another partition. > How can I mount a partition as a subdir of drive C? For some reason I can't: $ mount /cygdrive/e/Temp/dummy /cygdrive/c/Temp/dummy/dummyone mount: /cygdrive/c/Temp/dummy/dummyone: Invalid argument
Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt
+ istatus = NtQueryInformationFile (fh, &io, &qfbi, sizeof qfbi,
>> +FileBasicInformation);
>> + debug_printf ("NtQueryFileBasicInformation %S: %y",
>> +attr->ObjectName, istatus);
>> +
>> + if (NT_SUCCESS (istatus))
>> +{
>> + if (qfbi.FileAttributes & FILE_ATTRIBUTE_READONLY)
>> +{
>> + //Step 5
>> + //Remove the readonly flag
>> + FILE_BASIC_INFORMATION sfbi = { 0 };
>> + sfbi.FileAttributes = FILE_ATTRIBUTE_ARCHIVE;
>> + istatus = NtSetInformationFile (fh, &io, &sfbi,
>> + sizeof sfbi,
>> + FileBasicInformation);
>> + debug_printf ("NtSetFileBasicInformation %S: %y",
>> +attr->ObjectName, istatus);
>> +}
>> +
>> + if (NT_SUCCESS (istatus))
>> +{
>> + //Step 6a
>> + //Now, mark the file ready for deletion
>> + if (has_posix_unlink_semantics)
>> +{
>> + FILE_DISPOSITION_INFORMATION_EX fdie =
>> +{ FILE_DISPOSITION_DELETE
>> +| FILE_DISPOSITION_POSIX_SEMANTICS };
>> + istatus = NtSetInformationFile (
>> + fh, &io, &fdie, sizeof fdie,
>> + FileDispositionInformationEx);
>> + debug_printf (
>> + "NtSetFileDispositionInformationEx %S: %y",
>> + attr->ObjectName, istatus);
>> +
>> + if (istatus == STATUS_INVALID_PARAMETER)
>> +has_posix_unlink_semantics = false;
>> +}
>> +
>> + if (!has_posix_unlink_semantics
>> + || !NT_SUCCESS (istatus))
>> +{
>> + //Step 6b
>> + //This will remove a readonly file (as we have
>> just cleared that flag)
>> + //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.
Because even if the delete fails, the attributes are restored. Or,
very-unlikely-worst-case-scenario:
Both fail and we're left with a file with FILE_ATTRIBUTE_ARCHIVE which means
the file has been marked for deletion.
>> +static NTSTATUS
>> +_unlink_ntpc_ (path_conv& pc, bool shareable)
>
> The trailing underscore should be replaced with a descriptive postfix.
What about naming this function _unlink_nt and the original _unlink_nt,
_unlink_fallback?
>> +{
>> + OBJECT_ATTRIBUTES attr;
>> + ULONG eflags = pc.isdir () ? FILE_DIRECTORY_FILE :
>> FILE_NON_DIRECTORY_FILE;
>> +
>> + pc.get_object_attr (attr, sec_none_nih);
>> + NTSTATUS status = _unlink_nt (&attr, eflags);
>
> Sorry, but I don't grok that. You already have a path_conv, so what's
> the advantage of calling the unlink version without path_conv and thus,
> without certain check, like the reparse point check, or checks for
> filesystems with quirks, like MVFS? What about remote filesystems of
> which you don't know nothing, e. g., a Win7 NTFS? Did you check the
> error codes in this case?
>
The idea is to - eventually - replace/incorporate the fallback unlink_nt.
But, because I indeed don't know the quirks of all filesystems, I left the
fallback scenario intact, for now.
No, I have not checked all error codes, I simply don't have all filesystems at
my disposal, again the reason for keeping the fallback.
The general idea is to forgo path_conv's filesystem checks and just try to
delete the file,
if it fails, remember and fallback. After these series of commits, some will
follow
to try and see if we can remove/incorporate the fallback scenario completely.
Ben...
Re: [PATCH v2 7/8] dir.cc: Try unlink_nt first
On 26-01-2021 12:46, Corinna Vinschen via Cygwin-patches wrote: > > So what about /dev, /proc, etc? > The idea was to catch them with pc.isspecial() in unlink_nt.
[PATCH v3 2/8] syscalls.cc: Deduplicate remove
The remove code is already in the _remove_r function.
So, just call the _remove_r function.
---
winsup/cygwin/syscalls.cc | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 8651cfade..e6ff0fd7a 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1161,24 +1161,15 @@ _remove_r (struct _reent *, const char *ourname)
return -1;
}
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ syscall_printf ("%R = remove(%s)", res, ourname);
+ return res;
}
extern "C" int
remove (const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
-
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("-1 = remove (%s)", ourname);
- return -1;
-}
-
- int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
- syscall_printf ("%R = remove(%s)", res, ourname);
- return res;
+ return _remove_r (_REENT, ourname);
}
extern "C" pid_t
--
2.30.0
[PATCH v3 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
I think we don't need an extra flag as we can utilize: access &
FILE_WRITE_ATTRIBUTES
What do you think?
Ben Wijen (1):
syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
winsup/cygwin/ntdll.h | 3 ++-
winsup/cygwin/syscalls.cc | 22 +++
winsup/cygwin/wincap.cc | 11
winsup/cygwin/wincap.h| 56 ---
4 files changed, 53 insertions(+), 39 deletions(-)
--
2.30.0
>From 2d0ff6fec10d03c24d11c747852018b7bc1136ac Mon Sep 17 00:00:00 2001
In-Reply-To: <20210122105201.gd810...@calimero.vinschen.de>
References: <20210122105201.gd810...@calimero.vinschen.de>
From: Ben Wijen
Date: Tue, 17 Dec 2019 15:15:25 +0100
Subject: [PATCH v3 1/8] syscalls.cc: unlink_nt: Try
FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
Implement wincap.has_posix_unlink_semantics_with_ignore_readonly and when set
skip setting/clearing of READONLY attribute and instead use
FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
---
winsup/cygwin/ntdll.h | 3 ++-
winsup/cygwin/syscalls.cc | 22 +++
winsup/cygwin/wincap.cc | 11
winsup/cygwin/wincap.h| 56 ---
4 files changed, 53 insertions(+), 39 deletions(-)
diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h
index d4f6aaf45..7eee383dd 100644
--- a/winsup/cygwin/ntdll.h
+++ b/winsup/cygwin/ntdll.h
@@ -497,7 +497,8 @@ enum {
FILE_DISPOSITION_DELETE = 0x01,
FILE_DISPOSITION_POSIX_SEMANTICS = 0x02,
FILE_DISPOSITION_FORCE_IMAGE_SECTION_CHECK = 0x04,
- FILE_DISPOSITION_ON_CLOSE= 0x08
+ FILE_DISPOSITION_ON_CLOSE= 0x08,
+ FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE = 0x10,
};
enum
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 7044ea903..8651cfade 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -719,7 +719,7 @@ _unlink_nt (path_conv &pc, bool shareable)
pc.get_object_attr (attr, sec_none_nih);
- /* First check if we can use POSIX unlink semantics: W10 1709++, local NTFS.
+ /* First check if we can use POSIX unlink semantics: W10 1709+, local NTFS.
With POSIX unlink semantics the entire job gets MUCH easier and faster.
Just try to do it and if it fails, it fails. */
if (wincap.has_posix_unlink_semantics ()
@@ -727,20 +727,18 @@ _unlink_nt (path_conv &pc, bool shareable)
{
FILE_DISPOSITION_INFORMATION_EX fdie;
- if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ /* POSIX unlink semantics are nice, but they still fail if the file has
+the R/O attribute set. If so, ignoring might be an option: W10 1809+
+Removing the file is very much a safe bet afterwards, so, no
+transaction. */
+ if ((pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ && !wincap.has_posix_unlink_semantics_with_ignore_readonly ())
access |= FILE_WRITE_ATTRIBUTES;
status = NtOpenFile (&fh, access, &attr, &io, FILE_SHARE_VALID_FLAGS,
flags);
if (!NT_SUCCESS (status))
goto out;
- /* Why didn't the devs add a FILE_DELETE_IGNORE_READONLY_ATTRIBUTE
-flag just like they did with FILE_LINK_IGNORE_READONLY_ATTRIBUTE
-and FILE_LINK_IGNORE_READONLY_ATTRIBUTE???
-
- POSIX unlink semantics are nice, but they still fail if the file
-has the R/O attribute set. Removing the file is very much a safe
-bet afterwards, so, no transaction. */
- if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ if (access & FILE_WRITE_ATTRIBUTES)
{
status = NtSetAttributesFile (fh, pc.file_attributes ()
& ~FILE_ATTRIBUTE_READONLY);
@@ -751,10 +749,12 @@ _unlink_nt (path_conv &pc, bool shareable)
}
}
fdie.Flags = FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS;
+ if (wincap.has_posix_unlink_semantics_with_ignore_readonly ())
+fdie.Flags |= FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE;
status = NtSetInformationFile (fh, &io, &fdie, sizeof fdie,
FileDispositionInformationEx);
/* Restore R/O attribute in case we have multiple hardlinks. */
- if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ if (access & FILE_WRITE_ATTRIBUTES)
NtSetAttributesFile (fh, pc.file_attributes ());
NtClose (fh);
/* Trying to delete in-use executables and DLLs using
diff --git a/winsup/cygwin/wincap.cc b/winsup/cygwin/wincap.cc
index b18e732cd..635e0892b 100644
--- a/winsup/cygwin/wincap.cc
+++ b/winsup/cygwin/wincap.cc
@@ -38,6 +38,7 @@ wincaps wincap_vista __attribute__((section
(".cygwin_dll_common"), shared)) = {
has_
[PATCH v2 6/8] syscalls.cc: Expose shallow-pathconv unlink_nt
Not having to query file information improves unlink speed.
---
winsup/cygwin/syscalls.cc | 78 ++-
1 file changed, 52 insertions(+), 26 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index ab0c4c2d6..b5ab6ac5e 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1272,6 +1272,28 @@ _unlink_ntpc_ (path_conv& pc, bool shareable)
return status;
}
+NTSTATUS
+unlink_nt (const char *ourname, ULONG eflags)
+{
+ uint32_t opt = PC_SYM_NOFOLLOW | PC_SKIP_SYM_CHECK | PC_SKIP_FS_CHECK;
+ if (!(eflags & FILE_NON_DIRECTORY_FILE))
+opt &= ~PC_SKIP_FS_CHECK;
+
+ path_conv pc (ourname, opt, NULL);
+ if (pc.error || pc.isspecial ())
+return STATUS_CANNOT_DELETE;
+
+ OBJECT_ATTRIBUTES attr;
+ PUNICODE_STRING ntpath = pc.get_nt_native_path ();
+ InitializeObjectAttributes(&attr, ntpath, 0, NULL, NULL);
+ NTSTATUS status = _unlink_nt (&attr, eflags);
+
+ if (!(eflags & FILE_NON_DIRECTORY_FILE))
+status = _unlink_nt_post_dir_check (status, &attr, pc);
+
+ return status;
+}
+
NTSTATUS
unlink_ntpc (path_conv &pc)
{
@@ -1289,37 +1311,41 @@ unlink (const char *ourname)
{
int res = -1;
dev_t devn;
- NTSTATUS status;
+ NTSTATUS status = unlink_nt (ourname, FILE_NON_DIRECTORY_FILE);
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW, stat_suffixes);
+ if (!NT_SUCCESS (status))
+ {
+path_conv win32_name (ourname, PC_SYM_NOFOLLOW, stat_suffixes);
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- goto done;
-}
+if (win32_name.error)
+ {
+set_errno (win32_name.error);
+goto done;
+ }
- devn = win32_name.get_device ();
- if (isproc_dev (devn))
-{
- set_errno (EROFS);
- goto done;
-}
+devn = win32_name.get_device ();
+if (isproc_dev (devn))
+ {
+set_errno (EROFS);
+goto done;
+ }
- if (!win32_name.exists ())
-{
- debug_printf ("unlinking a nonexistent file");
- set_errno (ENOENT);
- goto done;
-}
- else if (win32_name.isdir ())
-{
- debug_printf ("unlinking a directory");
- set_errno (EISDIR);
- goto done;
-}
+if (!win32_name.exists ())
+ {
+debug_printf ("unlinking a nonexistent file");
+set_errno (ENOENT);
+goto done;
+ }
+else if (win32_name.isdir ())
+ {
+debug_printf ("unlinking a directory");
+set_errno (EISDIR);
+goto done;
+ }
+
+status = unlink_ntpc (win32_name);
+ }
- status = unlink_ntpc (win32_name);
if (NT_SUCCESS (status))
res = 0;
else
--
2.30.0
[PATCH v2 8/8] fhandler_disk_file.cc: Use path_conv's IndexNumber
path_conv already knows the IndexNumber, so just use it.
This commit also fixes the potential handle leak.
---
winsup/cygwin/fhandler_disk_file.cc | 24 ++--
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index fe04f832b..39f914a59 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -2029,9 +2029,6 @@ readdir_get_ino (const char *path, bool dot_dot)
{
char *fname;
struct stat st;
- HANDLE hdl;
- OBJECT_ATTRIBUTES attr;
- IO_STATUS_BLOCK io;
ino_t ino = 0;
if (dot_dot)
@@ -2044,26 +2041,17 @@ readdir_get_ino (const char *path, bool dot_dot)
path = fname;
}
path_conv pc (path, PC_SYM_NOFOLLOW | PC_POSIX | PC_KEEP_HANDLE);
- if (pc.isspecial ())
+ if (pc.isgood_inode (pc.fai ()->InternalInformation.IndexNumber.QuadPart))
+ino = pc.fai ()->InternalInformation.IndexNumber.QuadPart;
+ else if (pc.isspecial ())
{
if (!stat_worker (pc, &st))
ino = st.st_ino;
}
- else if (!pc.hasgood_inode ())
+
+ if (!ino)
ino = hash_path_name (0, pc.get_nt_native_path ());
- else if ((hdl = pc.handle ()) != NULL
- || NT_SUCCESS (NtOpenFile (&hdl, READ_CONTROL,
- pc.get_object_attr (attr, sec_none_nih),
- &io, FILE_SHARE_VALID_FLAGS,
- FILE_OPEN_FOR_BACKUP_INTENT
- | (pc.is_known_reparse_point ()
- ? FILE_OPEN_REPARSE_POINT : 0)))
- )
-{
- ino = pc.get_ino_by_handle (hdl);
- if (!ino)
- ino = hash_path_name (0, pc.get_nt_native_path ());
-}
+
return ino;
}
--
2.30.0
[PATCH v2 7/8] dir.cc: Try unlink_nt first
Speedup deletion of directories.
---
winsup/cygwin/dir.cc | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index 7762557d6..470f83aee 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -22,6 +22,8 @@ details. */
#include "cygtls.h"
#include "tls_pbuf.h"
+extern NTSTATUS unlink_nt (const char *ourname, ULONG eflags);
+
extern "C" int
dirfd (DIR *dir)
{
@@ -398,6 +400,14 @@ rmdir (const char *dir)
if (msdos && p == dir + 1 && isdrive (dir))
p[1] = '\\';
}
+ if (has_dot_last_component (dir, false)) {
+set_errno (EINVAL);
+__leave;
+ }
+ if (NT_SUCCESS (unlink_nt (dir, FILE_DIRECTORY_FILE))) {
+res = 0;
+__leave;
+ }
if (!(fh = build_fh_name (dir, PC_SYM_NOFOLLOW)))
__leave; /* errno already set */;
@@ -408,8 +418,6 @@ rmdir (const char *dir)
}
else if (!fh->exists ())
set_errno (ENOENT);
- else if (has_dot_last_component (dir, false))
- set_errno (EINVAL);
else if (!fh->rmdir ())
res = 0;
delete fh;
--
2.30.0
[PATCH v2 5/8] path.cc: Allow to skip filesystem checks
When file attributes are of no concern, there is no point to query them.
This can greatly speedup code which doesn't need it.
The idea is to have a shallow path conversion with only minimal information.
The upcoming unlink_nt for example, first tries a path without filesystem
checks, then - if necessary - retries with filesystem checks.
---
winsup/cygwin/path.cc | 7 ---
winsup/cygwin/path.h | 2 ++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index abd3687df..441fe113b 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -627,7 +627,7 @@ path_conv::check (const char *src, unsigned opt,
char *pathbuf = tp.c_get ();
char *tmp_buf = tp.t_get ();
char *THIS_path = tp.c_get ();
- symlink_info sym;
+ symlink_info sym = { 0 };
bool need_directory = 0;
bool add_ext = false;
bool is_relpath;
@@ -931,7 +931,8 @@ path_conv::check (const char *src, unsigned opt,
is_fs_via_procsys:
- symlen = sym.check (full_path, suff, fs, conv_handle);
+ if (!(opt & PC_SKIP_SYM_CHECK))
+ symlen = sym.check (full_path, suff, fs, conv_handle);
is_virtual_symlink:
@@ -1172,7 +1173,7 @@ path_conv::check (const char *src, unsigned opt,
return;
}
- if (dev.isfs ())
+ if (!(opt & PC_SKIP_FS_CHECK) && dev.isfs ())
{
/* If FS hasn't been checked already in symlink_info::check,
do so now. */
diff --git a/winsup/cygwin/path.h b/winsup/cygwin/path.h
index 62bd5ddd5..5821cdf57 100644
--- a/winsup/cygwin/path.h
+++ b/winsup/cygwin/path.h
@@ -59,6 +59,8 @@ enum pathconv_arg
PC_KEEP_HANDLE= _BIT (12), /* keep handle for later stat calls */
PC_NO_ACCESS_CHECK= _BIT (13), /* helper flag for error check */
PC_SYM_NOFOLLOW_DIR = _BIT (14), /* don't follow a trailing slash */
+ PC_SKIP_SYM_CHECK = _BIT (15), /* skip symlink_info::check */
+ PC_SKIP_FS_CHECK = _BIT (16), /* skip fs::update check */
PC_DONT_USE = _BIT (31)/* conversion to signed happens. */
};
--
2.30.0
[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 &pc);
+ extern NTSTATUS unlink_ntpc (path_conv &pc);
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 &pc);
+extern NTSTATUS unlink_ntpc_shareable (path_conv &pc);
#define MUTEXSEP L"@"
#define PATHSEP L"\\"
@@ -132,7 +132,7 @@ rmdirs (WCHAR ntmaxpathbuf[NT_MAX_PATH])
RtlInitUnicodeString (&fn, ntmaxpathbuf);
path_conv pc (&fn);
- 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 &pc, HANDLE &fh, 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 &pc, HANDLE &fh, ACCESS_MASK access,
ULONG flags)
if (NT_SUCCESS (NtSetInformationFile (fh, &io, 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 &pc, HANDLE &fh, 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 &pc, 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 (&fh, access, attr, &io, 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)// Th
[PATCH v2 3/8] Cygwin: Move post-dir unlink check
Move post-dir unlink check from fhandler_disk_file::rmdir to
_unlink_nt_post_dir_check
If a directory is not removed through fhandler_disk_file::rmdir
we can now make sure the post dir check is performed.
---
winsup/cygwin/fhandler_disk_file.cc | 20
winsup/cygwin/syscalls.cc | 28
2 files changed, 28 insertions(+), 20 deletions(-)
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index 885b59161..07f9c513a 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1852,26 +1852,6 @@ fhandler_disk_file::rmdir ()
NTSTATUS status = unlink_nt (pc);
- /* Check for existence of remote dirs after trying to delete them.
- Two reasons:
- - Sometimes SMB indicates failure when it really succeeds.
- - Removing a directory on a Samba drive using an old Samba version
- sometimes doesn't return an error, if the directory can't be removed
- because it's not empty. */
- if (isremote ())
-{
- OBJECT_ATTRIBUTES attr;
- FILE_BASIC_INFORMATION fbi;
- NTSTATUS q_status;
-
- q_status = NtQueryAttributesFile (pc.get_object_attr (attr,
sec_none_nih),
- &fbi);
- if (!NT_SUCCESS (status) && q_status == STATUS_OBJECT_NAME_NOT_FOUND)
- status = STATUS_SUCCESS;
- else if (pc.fs_is_samba ()
- && NT_SUCCESS (status) && NT_SUCCESS (q_status))
- status = STATUS_DIRECTORY_NOT_EMPTY;
-}
if (!NT_SUCCESS (status))
{
__seterrno_from_nt_status (status);
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 54b065733..b220bedae 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -670,6 +670,30 @@ check_dir_not_empty (HANDLE dir, path_conv &pc)
return STATUS_SUCCESS;
}
+static NTSTATUS
+_unlink_nt_post_dir_check (NTSTATUS status, POBJECT_ATTRIBUTES attr, const
path_conv &pc)
+{
+ /* Check for existence of remote dirs after trying to delete them.
+ Two reasons:
+ - Sometimes SMB indicates failure when it really succeeds.
+ - Removing a directory on a Samba drive using an old Samba version
+ sometimes doesn't return an error, if the directory can't be removed
+ because it's not empty. */
+ if (pc.isremote ())
+{
+ FILE_BASIC_INFORMATION fbi;
+ NTSTATUS q_status;
+
+ q_status = NtQueryAttributesFile (attr, &fbi);
+ if (!NT_SUCCESS (status) && q_status == STATUS_OBJECT_NAME_NOT_FOUND)
+ status = STATUS_SUCCESS;
+ else if (pc.fs_is_samba ()
+ && NT_SUCCESS (status) && NT_SUCCESS (q_status))
+ status = STATUS_DIRECTORY_NOT_EMPTY;
+}
+ return status;
+}
+
static NTSTATUS
_unlink_nt (path_conv &pc, bool shareable)
{
@@ -1059,6 +1083,10 @@ out:
/* Stop transaction if we started one. */
if (trans)
stop_transaction (status, old_trans, trans);
+
+ if (pc.isdir ())
+status = _unlink_nt_post_dir_check (status, &attr, pc);
+
syscall_printf ("%S, return status = %y", pc.get_nt_native_path (), status);
return status;
}
--
2.30.0
[PATCH v2 2/8] syscalls.cc: Deduplicate remove
The remove code is already in the _remove_r function.
So, just call the _remove_r function.
---
winsup/cygwin/syscalls.cc | 17 -
1 file changed, 4 insertions(+), 13 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 2e50ad7d5..54b065733 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1133,24 +1133,15 @@ _remove_r (struct _reent *, const char *ourname)
return -1;
}
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ syscall_printf ("%R = remove(%s)", res, ourname);
+ return res;
}
extern "C" int
remove (const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
-
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("-1 = remove (%s)", ourname);
- return -1;
-}
-
- int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
- syscall_printf ("%R = remove(%s)", res, ourname);
- return res;
+ return _remove_r(_REENT, ourname);
}
extern "C" pid_t
--
2.30.0
[PATCH v2 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
Implement wincap.has_posix_unlink_semantics_with_ignore_readonly and when set
skip setting/clearing of READONLY attribute and instead use
FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE
---
winsup/cygwin/ntdll.h | 3 ++-
winsup/cygwin/syscalls.cc | 14 +-
winsup/cygwin/wincap.cc | 11
winsup/cygwin/wincap.h| 56 ---
4 files changed, 49 insertions(+), 35 deletions(-)
diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h
index d4f6aaf45..7eee383dd 100644
--- a/winsup/cygwin/ntdll.h
+++ b/winsup/cygwin/ntdll.h
@@ -497,7 +497,8 @@ enum {
FILE_DISPOSITION_DELETE = 0x01,
FILE_DISPOSITION_POSIX_SEMANTICS = 0x02,
FILE_DISPOSITION_FORCE_IMAGE_SECTION_CHECK = 0x04,
- FILE_DISPOSITION_ON_CLOSE= 0x08
+ FILE_DISPOSITION_ON_CLOSE= 0x08,
+ FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE = 0x10,
};
enum
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4742c6653..2e50ad7d5 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -709,14 +709,11 @@ _unlink_nt (path_conv &pc, bool shareable)
flags);
if (!NT_SUCCESS (status))
goto out;
- /* Why didn't the devs add a FILE_DELETE_IGNORE_READONLY_ATTRIBUTE
-flag just like they did with FILE_LINK_IGNORE_READONLY_ATTRIBUTE
-and FILE_LINK_IGNORE_READONLY_ATTRIBUTE???
-
- POSIX unlink semantics are nice, but they still fail if the file
+ /* POSIX unlink semantics are nice, but they still fail if the file
has the R/O attribute set. Removing the file is very much a safe
bet afterwards, so, no transaction. */
- if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ if (!wincap.has_posix_unlink_semantics_with_ignore_readonly ()
+ && (pc.file_attributes () & FILE_ATTRIBUTE_READONLY))
{
status = NtSetAttributesFile (fh, pc.file_attributes ()
& ~FILE_ATTRIBUTE_READONLY);
@@ -727,10 +724,13 @@ _unlink_nt (path_conv &pc, bool shareable)
}
}
fdie.Flags = FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS;
+ if(wincap.has_posix_unlink_semantics_with_ignore_readonly ())
+ fdie.Flags |= FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE;
status = NtSetInformationFile (fh, &io, &fdie, sizeof fdie,
FileDispositionInformationEx);
/* Restore R/O attribute in case we have multiple hardlinks. */
- if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
+ if (!wincap.has_posix_unlink_semantics_with_ignore_readonly ()
+ && (pc.file_attributes () & FILE_ATTRIBUTE_READONLY))
NtSetAttributesFile (fh, pc.file_attributes ());
NtClose (fh);
/* Trying to delete in-use executables and DLLs using
diff --git a/winsup/cygwin/wincap.cc b/winsup/cygwin/wincap.cc
index b18e732cd..635e0892b 100644
--- a/winsup/cygwin/wincap.cc
+++ b/winsup/cygwin/wincap.cc
@@ -38,6 +38,7 @@ wincaps wincap_vista __attribute__((section
(".cygwin_dll_common"), shared)) = {
has_unbiased_interrupt_time:false,
has_precise_interrupt_time:false,
has_posix_unlink_semantics:false,
+has_posix_unlink_semantics_with_ignore_readonly:false,
has_case_sensitive_dirs:false,
has_posix_rename_semantics:false,
no_msv1_0_s4u_logon_in_wow64:true,
@@ -72,6 +73,7 @@ wincaps wincap_7 __attribute__((section
(".cygwin_dll_common"), shared)) = {
has_unbiased_interrupt_time:true,
has_precise_interrupt_time:false,
has_posix_unlink_semantics:false,
+has_posix_unlink_semantics_with_ignore_readonly:false,
has_case_sensitive_dirs:false,
has_posix_rename_semantics:false,
no_msv1_0_s4u_logon_in_wow64:true,
@@ -106,6 +108,7 @@ wincaps wincap_8 __attribute__((section
(".cygwin_dll_common"), shared)) = {
has_unbiased_interrupt_time:true,
has_precise_interrupt_time:false,
has_posix_unlink_semantics:false,
+has_posix_unlink_semantics_with_ignore_readonly:false,
has_case_sensitive_dirs:false,
has_posix_rename_semantics:false,
no_msv1_0_s4u_logon_in_wow64:false,
@@ -140,6 +143,7 @@ wincaps wincap_8_1 __attribute__((section
(".cygwin_dll_common"), shared)) = {
has_unbiased_interrupt_time:true,
has_precise_interrupt_time:false,
has_posix_unlink_semantics:false,
+has_posix_unlink_semantics_with_ignore_readonly:false,
has_case_sensitive_dirs:false,
has_posix_rename_semantics:false,
no_msv1_0_s4u_logon_in_wow64:false,
@@ -174,6 +178,7 @@ wincaps wincap_10_1507 __attribute__((section
(".cygwin_dll_common"), shared))
has_unbiased_interrupt_time:true,
has_precise_interrupt_time:true,
has_posix_unlink_semantics:false,
+has_posix_unlink_semantics_with_ignore_readonly:false,
has_case_sen
[PATCH v2 0/8] Improve rm speed
Hi, I think I got all remarks, please let me know if I missed something. I'm still thinking on a better way to use fs_info::update cache, but it requires more testing. Thank you, Ben Wijen (8): syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE syscalls.cc: Deduplicate remove Cygwin: Move post-dir unlink check syscalls.cc: Implement non-path_conv dependent _unlink_nt path.cc: Allow to skip filesystem checks syscalls.cc: Expose shallow-pathconv unlink_nt dir.cc: Try unlink_nt first fhandler_disk_file.cc: Use path_conv's IndexNumber winsup/cygwin/dir.cc| 12 +- winsup/cygwin/fhandler_disk_file.cc | 48 +--- winsup/cygwin/forkable.cc | 4 +- winsup/cygwin/ntdll.h | 3 +- winsup/cygwin/path.cc | 7 +- winsup/cygwin/path.h| 2 + winsup/cygwin/syscalls.cc | 346 +++- winsup/cygwin/wincap.cc | 11 + winsup/cygwin/wincap.h | 56 ++--- 9 files changed, 354 insertions(+), 135 deletions(-) -- 2.30.0
Re: [PATCH 11/11] dir.cc: Try unlink_nt first
On 18-01-2021 18:07, Ben wrote: > > Have I missed something else? > Alright, I now see unlink uses isproc_dev and rmdir uses isdev_dev. Is this correct? Why are files and directories handled differently? Anyway, I will add isdev_dev to the new unlink_nt Ben...
Re: [PATCH 08/11] path.cc: Allow to skip filesystem checks
On 18-01-2021 12:36, Corinna Vinschen via Cygwin-patches wrote: > On Jan 15 14:45, Ben Wijen wrote: > > Without any code setting the flag, this doesn't seem to make any > sense. At least the commit message should reflect on the reasons > for this change. > Something like this: path.cc: Allow to skip filesystem checks When file attributes are of no concern, there is no point to query them. This can greatly speedup code which doesn't need it. For example, this can be used to try a path without filesystem checks first and try again with filesystem checks If you want, I can also squash some of these related commits. Ben...
Re: [PATCH 11/11] dir.cc: Try unlink_nt first
On 18-01-2021 13:13, Corinna Vinschen via Cygwin-patches wrote: > > Your code is skipping the safety checks and the has_dot_last_component() > check. The latter implements a check required by POSIX. Skipping > it introduces an incompatibility, see man 2 rmdir. > Yes, I missed has_dot_last_component completely. As for the other checks: dir.cc: 404: fh->error (): * Done in unlink_nt dir.cc: 409: fh->exists (): * Done in _unlink_nt through NtOpenFile, which will return either STATUS_OBJECT_NAME_NOT_FOUND or STATUS_OBJECT_PATH_NOT_FOUND, both of which resolve to ENOENT dir.cc: 413: isdev_dev (fh->dev ()): * Done in unlink_nt fhandler_siak_file.cc: 1842: if (!pc.isdir ()) * Done in _unlink_nt through NtOpenFile with flags FILE_DIRECTORY_FILE and FILE_NON_DIRECTORY_FILE which will return STATUS_NOT_A_DIRECTORY and STATUS_FILE_IS_A_DIRECTORY respectively. Have I missed something else? Also, I think it's better to have isdev_dev (fh->dev ()) return EROFS, which is the same as unlink.
Re: [PATCH 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first
> > I'm sure, but that code path is called on non-remote ntfs only anyway. > Ofcourse, I was thinking about the new _unlink_nt...
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 15:49, Corinna Vinschen via Cygwin-patches wrote: > > Care to send the resulting patch? > Will send with next patch-set. Ben...
Re: [PATCH 05/11] Cygwin: Move post-dir unlink check
On 18-01-2021 12:08, Corinna Vinschen via Cygwin-patches wrote: > On Jan 15 14:45, Ben Wijen wrote: >> Move post-dir unlink check from >> fhandler_disk_file::rmdir to _unlink_nt > > Why? It's not much of a problem, codewise, but the commit message > could be improved here. > Something like this? Cygwin: Move post-dir unlink check Move post-dir unlink check from fhandler_disk_file::rmdir to _unlink_nt This helps in two ways: * Now all checks are in one place * Even if a directory is removed through _unlink_nt, but not rmdir, the return value can be trusted.
Re: [PATCH 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first
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 >
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 14:04, Corinna Vinschen via Cygwin-patches wrote:
> What about this instead? It should be better optimizable:
>
Hmmm:
* _remove_r should still set reent->_errno
* _GLOBAL_REENT isn't threadlocal, what about __getreent()
So maybe:
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4742c6653..9c498cd46 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1122,35 +1122,28 @@ unlink (const char *ourname)
}
extern "C" int
-_remove_r (struct _reent *, const char *ourname)
+_remove_r (struct _reent *ptr, const char *ourname)
{
path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
if (win32_name.error)
{
- set_errno (win32_name.error);
+ ptr->_errno = set_errno (win32_name.error);
syscall_printf ("%R = remove(%s)",-1, ourname);
return -1;
}
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ if (errno!=0)
+ ptr->_errno = errno;
+ syscall_printf ("%R = remove(%s)", res, ourname);
+ return res;
}
extern "C" int
remove (const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
-
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("-1 = remove (%s)", ourname);
- return -1;
-}
-
- int res = win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
- syscall_printf ("%R = remove(%s)", res, ourname);
- return res;
+ return _remove_r (__getreent (), ourname);
}
extern "C" pid_t
Ben...
Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r
On 18-01-2021 11:56, Corinna Vinschen via Cygwin-patches wrote: > Hmm, you're adding another function call to the call stack. Doesn't > that slow down _remove_r rather than speeding it up? Ok, this function > is called from _tmpfile_r/_tmpfile64_r only, so dedup may trump speed > here... > > What's your stance? > While I could do without: In an earlier version I had changed remove and missed remove_r. So, this commit is more about de-duplication rather than speed. Ben...
Re: [PATCH 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first
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? Also I have seen NtSetInformationFile fail with STATUS_INVALID_PARAMETER. So a retry without FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE is valid here. 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. Ben...
Re: [PATCH 07/11] syscalls.cc: Implement non-path_conv dependent _unlink_nt
Hi, After reiterating over my patches, I see this must come after implementing the 'poor mans cache' commit. I will reorder in a new patch-set. Sorry about that. Ben... On 15-01-2021 14:45, 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 | 239 ++-- > 3 files changed, 228 insertions(+), 19 deletions(-) >
[PATCH 11/11] dir.cc: Try unlink_nt first
Speedup deletion of directories.
---
winsup/cygwin/dir.cc | 6 ++
1 file changed, 6 insertions(+)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index f912a9e47..2e7da3638 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -22,6 +22,8 @@ details. */
#include "cygtls.h"
#include "tls_pbuf.h"
+extern NTSTATUS unlink_nt (const char *ourname, ULONG eflags);
+
extern "C" int
dirfd (DIR *dir)
{
@@ -398,6 +400,10 @@ rmdir (const char *dir)
if (msdos && p == dir + 1 && isdrive (dir))
p[1] = '\\';
}
+ if(NT_SUCCESS(unlink_nt (dir, FILE_DIRECTORY_FILE))) {
+res = 0;
+__leave;
+ }
if (!(fh = build_fh_name (dir, PC_SYM_NOFOLLOW)))
__leave; /* errno already set */;
--
2.29.2
[PATCH 08/11] path.cc: Allow to skip filesystem checks
When file attributes are of no concern,
there is no point to query them.
---
winsup/cygwin/path.cc | 3 +++
winsup/cygwin/path.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index abd3687df..f00707e86 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -931,7 +931,10 @@ path_conv::check (const char *src, unsigned opt,
is_fs_via_procsys:
+ if (!(opt & PC_SKIP_SYM_CHECK))
+ {
symlen = sym.check (full_path, suff, fs, conv_handle);
+ }
is_virtual_symlink:
diff --git a/winsup/cygwin/path.h b/winsup/cygwin/path.h
index 62bd5ddd5..56855e1c9 100644
--- a/winsup/cygwin/path.h
+++ b/winsup/cygwin/path.h
@@ -59,6 +59,7 @@ enum pathconv_arg
PC_KEEP_HANDLE= _BIT (12), /* keep handle for later stat calls */
PC_NO_ACCESS_CHECK= _BIT (13), /* helper flag for error check */
PC_SYM_NOFOLLOW_DIR = _BIT (14), /* don't follow a trailing slash */
+ PC_SKIP_SYM_CHECK = _BIT (15), /* skip symlink_info::check */
PC_DONT_USE = _BIT (31)/* conversion to signed happens. */
};
--
2.29.2
[PATCH 09/11] mount.cc: Implement poor-man's cache
Try to avoid NtQueryVolumeInformationFile.
---
winsup/cygwin/mount.cc | 78 --
winsup/cygwin/mount.h | 2 +-
winsup/cygwin/path.cc | 2 +-
winsup/cygwin/path.h | 1 +
4 files changed, 56 insertions(+), 27 deletions(-)
diff --git a/winsup/cygwin/mount.cc b/winsup/cygwin/mount.cc
index e0349815d..1d2b3a61a 100644
--- a/winsup/cygwin/mount.cc
+++ b/winsup/cygwin/mount.cc
@@ -82,6 +82,32 @@ win32_device_name (const char *src_path, char *win32_path,
device& dev)
return true;
}
+static uint32_t
+hash_prefix (const PUNICODE_STRING upath)
+{
+ UNICODE_STRING prefix;
+ WCHAR *p;
+
+ if (upath->Buffer[5] == L':' && upath->Buffer[6] == L'\\')
+p = upath->Buffer + 6;
+ else
+{
+ /* We're expecting an UNC path. Move p to the backslash after
+ "\??\UNC\server\share" or the trailing NUL. */
+ p = upath->Buffer + 7; /* Skip "\??\UNC" */
+ int bs_cnt = 0;
+
+ while (*++p)
+if (*p == L'\\')
+ if (++bs_cnt > 1)
+break;
+}
+ RtlInitCountedUnicodeString (&prefix, upath->Buffer,
+ (p - upath->Buffer) * sizeof(WCHAR));
+
+ return hash_path_name ((ino_t) 0, &prefix);
+}
+
/* Beginning with Samba 3.0.28a, Samba allows to get version information using
the ExtendedInfo member returned by a FileFsObjectIdInformation request.
We just store the samba_version information for now. Older versions than
@@ -106,14 +132,16 @@ class fs_info_cache
struct {
fs_info fsi;
uint32_t hash;
+uint32_t prefix_hash;
} entry[MAX_FS_INFO_CNT];
uint32_t genhash (PFILE_FS_VOLUME_INFORMATION);
public:
fs_info_cache () : count (0) { fsi_lock.init ("fsi_lock"); }
+ fs_info *search (uint32_t);
fs_info *search (PFILE_FS_VOLUME_INFORMATION, uint32_t &);
- void add (uint32_t, fs_info *);
+ void add (uint32_t, fs_info *, uint32_t);
};
static fs_info_cache fsi_cache;
@@ -142,22 +170,31 @@ fs_info_cache::search (PFILE_FS_VOLUME_INFORMATION pffvi,
uint32_t &hash)
return &entry[i].fsi;
return NULL;
}
+fs_info*
+fs_info_cache::search (uint32_t prefix_hash)
+{
+ for (uint32_t i = 0; i < count; ++i)
+if (entry[i].prefix_hash == prefix_hash)
+ return &entry[i].fsi;
+ return NULL;
+}
void
-fs_info_cache::add (uint32_t hashval, fs_info *new_fsi)
+fs_info_cache::add (uint32_t hashval, fs_info *new_fsi, uint32_t prefix_hash)
{
fsi_lock.acquire ();
if (count < MAX_FS_INFO_CNT)
{
entry[count].fsi = *new_fsi;
entry[count].hash = hashval;
+ entry[count].prefix_hash = prefix_hash;
++count;
}
fsi_lock.release ();
}
bool
-fs_info::update (PUNICODE_STRING upath, HANDLE in_vol)
+fs_info::update (PUNICODE_STRING upath, HANDLE in_vol, bool use_prefix_hash)
{
NTSTATUS status = STATUS_OBJECT_NAME_NOT_FOUND;
HANDLE vol;
@@ -178,6 +215,17 @@ fs_info::update (PUNICODE_STRING upath, HANDLE in_vol)
UNICODE_STRING fsname;
clear ();
+
+ if (use_prefix_hash)
+{
+ fs_info *fsi = fsi_cache.search (hash_prefix (upath));
+ if (fsi)
+{
+ *this = *fsi;
+ return true;
+}
+}
+
/* Always caseinsensitive. We really just need access to the drive. */
InitializeObjectAttributes (&attr, upath, OBJ_CASE_INSENSITIVE, NULL, NULL);
if (in_vol)
@@ -233,27 +281,7 @@ fs_info::update (PUNICODE_STRING upath, HANDLE in_vol)
a unique per-drive/share hash. */
if (ffvi_buf.ffvi.VolumeSerialNumber == 0)
{
- UNICODE_STRING path_prefix;
- WCHAR *p;
-
- if (upath->Buffer[5] == L':' && upath->Buffer[6] == L'\\')
- p = upath->Buffer + 6;
- else
- {
- /* We're expecting an UNC path. Move p to the backslash after
-"\??\UNC\server\share" or the trailing NUL. */
- p = upath->Buffer + 7; /* Skip "\??\UNC" */
- int bs_cnt = 0;
-
- while (*++p)
- if (*p == L'\\')
- if (++bs_cnt > 1)
- break;
- }
- RtlInitCountedUnicodeString (&path_prefix, upath->Buffer,
- (p - upath->Buffer) * sizeof (WCHAR));
- ffvi_buf.ffvi.VolumeSerialNumber = hash_path_name ((ino_t) 0,
-&path_prefix);
+ ffvi_buf.ffvi.VolumeSerialNumber = hash_prefix(upath);
}
fs_info *fsi = fsi_cache.search (&ffvi_buf.ffvi, hash);
if (fsi)
@@ -460,7 +488,7 @@ fs_info::update (PUNICODE_STRING upath, HANDLE in_vol)
if (!in_vol)
NtClose (vol);
- fsi_cache.add (hash, this);
+ fsi_cache.add (hash, this, hash_prefix (upath));
return true;
}
diff --git a/winsup/cygwin/mount.h b/winsup/cygwin/mount.h
index 122a679a8..86b72fb4c 100644
--- a/winsup/cygwin/mount.h
+++ b/winsup/cygwin/mount.h
@@ -124,7 +124,7 @@ class fs_info
const char *fsname () const { return fsn[0] ? f
[PATCH 10/11] syscalls.cc: Expose shallow-pathconv unlink_nt
Not having to query file information improves unlink speed.
---
winsup/cygwin/syscalls.cc | 68 ---
1 file changed, 42 insertions(+), 26 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 79e4a4422..8aecdf453 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1305,6 +1305,18 @@ _unlink_ntpc_ (path_conv& pc, bool shareable)
return status;
}
+NTSTATUS
+unlink_nt (const char *ourname, ULONG eflags)
+{
+ path_conv pc (ourname, PC_SYM_NOFOLLOW | PC_FS_USE_PREFIX_HASH |
PC_SKIP_SYM_CHECK, NULL);
+ dev_t devn = pc.get_device ();
+ if (pc.error || isproc_dev (devn))
+return STATUS_CANNOT_DELETE;
+
+ PUNICODE_STRING ntpath = pc.get_nt_native_path ();
+ return _unlink_nt (ntpath, eflags, 0);
+}
+
NTSTATUS
unlink_ntpc (path_conv &pc)
{
@@ -1322,37 +1334,41 @@ unlink (const char *ourname)
{
int res = -1;
dev_t devn;
- NTSTATUS status;
+ NTSTATUS status = unlink_nt (ourname, FILE_NON_DIRECTORY_FILE);
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW, stat_suffixes);
+ if (!NT_SUCCESS (status))
+ {
+path_conv win32_name (ourname, PC_SYM_NOFOLLOW, stat_suffixes);
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- goto done;
-}
+if (win32_name.error)
+ {
+set_errno (win32_name.error);
+goto done;
+ }
- devn = win32_name.get_device ();
- if (isproc_dev (devn))
-{
- set_errno (EROFS);
- goto done;
-}
+devn = win32_name.get_device ();
+if (isproc_dev (devn))
+ {
+set_errno (EROFS);
+goto done;
+ }
- if (!win32_name.exists ())
-{
- debug_printf ("unlinking a nonexistent file");
- set_errno (ENOENT);
- goto done;
-}
- else if (win32_name.isdir ())
-{
- debug_printf ("unlinking a directory");
- set_errno (EISDIR);
- goto done;
-}
+if (!win32_name.exists ())
+ {
+debug_printf ("unlinking a nonexistent file");
+set_errno (ENOENT);
+goto done;
+ }
+else if (win32_name.isdir ())
+ {
+debug_printf ("unlinking a directory");
+set_errno (EISDIR);
+goto done;
+ }
+
+status = unlink_ntpc (win32_name);
+ }
- status = unlink_ntpc (win32_name);
if (NT_SUCCESS (status))
res = 0;
else
--
2.29.2
[PATCH 06/11] cxx.cc: Fix dynamic initialization for static local variables
The old implementation for __cxa_guard_acquire did not return 1,
therefore dynamic initialization was never performed.
If concurrent-safe dynamic initialisation is ever needed, CXX ABI
must be followed when re-implementing __cxa_guard_acquire (et al.)
---
winsup/cygwin/Makefile.in | 2 +-
winsup/cygwin/cxx.cc | 10 --
2 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/winsup/cygwin/Makefile.in b/winsup/cygwin/Makefile.in
index a840f2b83..73d9b37fd 100644
--- a/winsup/cygwin/Makefile.in
+++ b/winsup/cygwin/Makefile.in
@@ -69,7 +69,7 @@ COMMON_CFLAGS=-MMD ${$(*F)_CFLAGS} -Wimplicit-fallthrough=5
-Werror -fmerge-cons
ifeq ($(target_cpu),x86_64)
COMMON_CFLAGS+=-mcmodel=small
endif
-COMPILE.cc+=${COMMON_CFLAGS} # -std=gnu++14
+COMPILE.cc+=${COMMON_CFLAGS} -fno-threadsafe-statics # -std=gnu++14
COMPILE.c+=${COMMON_CFLAGS}
AR:=@AR@
diff --git a/winsup/cygwin/cxx.cc b/winsup/cygwin/cxx.cc
index be3268549..b69524aca 100644
--- a/winsup/cygwin/cxx.cc
+++ b/winsup/cygwin/cxx.cc
@@ -83,16 +83,6 @@ __cxa_pure_virtual (void)
api_fatal ("pure virtual method called");
}
-extern "C" void
-__cxa_guard_acquire ()
-{
-}
-
-extern "C" void
-__cxa_guard_release ()
-{
-}
-
/* These routines are made available as last-resort fallbacks
for the application. Should not be used in practice; the
entries in this struct get overwritten by each DLL as it
--
2.29.2
[PATCH 07/11] 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 | 239 ++--
3 files changed, 228 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 &pc);
+ extern NTSTATUS unlink_ntpc (path_conv &pc);
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 &pc);
+extern NTSTATUS unlink_ntpc_shareable (path_conv &pc);
#define MUTEXSEP L"@"
#define PATHSEP L"\\"
@@ -132,7 +132,7 @@ rmdirs (WCHAR ntmaxpathbuf[NT_MAX_PATH])
RtlInitUnicodeString (&fn, ntmaxpathbuf);
path_conv pc (&fn);
- 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 f86a93825..79e4a4422 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -491,7 +491,7 @@ try_to_bin (path_conv &pc, HANDLE &fh, 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 &pc, HANDLE &fh, ACCESS_MASK access,
ULONG flags)
if (NT_SUCCESS (NtSetInformationFile (fh, &io, 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 &pc, HANDLE &fh, 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 ())
@@ -671,7 +671,187 @@ check_dir_not_empty (HANDLE dir, path_conv &pc)
}
static NTSTATUS
-_unlink_nt (path_conv &pc, bool shareable)
+_unlink_nt (PUNICODE_STRING ntpath, ULONG eflags, ULONG oattr)
+{
+ //Available as of Redstone 1 (Win10_17_09)
+ static bool has_posix_unlink_semantics =
+ wincap.has_posix_unlink_semantics ();
+ //Available as of Redstone 5 (Win10_18_09) (As were POSIX rename semantics)
+ static bool has_posix_unlink_semantics_with_ignore_readonly =
+ wincap.has_posix_rename_semantics ();
+
+ HANDLE fh;
+ ACCESS_MASK access = DELETE;
+ OBJECT_ATTRIBUTES attr;
+ 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", ntpath,
+ eflags == FILE_DIRECTORY_FILE);
+
+ InitializeObjectAttributes(&attr, ntpath, oattr, NULL, NULL);
+
+ //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 (&fh, access, &attr, &io, FILE_SHARE_DELETE, flags);
+ debug_printf("NtOpenFile %S: %y", ntpath, fstatus);
+}
+
+ if (!(eflags & FILE_NON_DIRECTORY_FILE) || // Worka
[PATCH 05/11] Cygwin: Move post-dir unlink check
Move post-dir unlink check from
fhandler_disk_file::rmdir to _unlink_nt
---
winsup/cygwin/fhandler_disk_file.cc | 20
winsup/cygwin/syscalls.cc | 21 +
2 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/winsup/cygwin/fhandler_disk_file.cc
b/winsup/cygwin/fhandler_disk_file.cc
index 885b59161..07f9c513a 100644
--- a/winsup/cygwin/fhandler_disk_file.cc
+++ b/winsup/cygwin/fhandler_disk_file.cc
@@ -1852,26 +1852,6 @@ fhandler_disk_file::rmdir ()
NTSTATUS status = unlink_nt (pc);
- /* Check for existence of remote dirs after trying to delete them.
- Two reasons:
- - Sometimes SMB indicates failure when it really succeeds.
- - Removing a directory on a Samba drive using an old Samba version
- sometimes doesn't return an error, if the directory can't be removed
- because it's not empty. */
- if (isremote ())
-{
- OBJECT_ATTRIBUTES attr;
- FILE_BASIC_INFORMATION fbi;
- NTSTATUS q_status;
-
- q_status = NtQueryAttributesFile (pc.get_object_attr (attr,
sec_none_nih),
- &fbi);
- if (!NT_SUCCESS (status) && q_status == STATUS_OBJECT_NAME_NOT_FOUND)
- status = STATUS_SUCCESS;
- else if (pc.fs_is_samba ()
- && NT_SUCCESS (status) && NT_SUCCESS (q_status))
- status = STATUS_DIRECTORY_NOT_EMPTY;
-}
if (!NT_SUCCESS (status))
{
__seterrno_from_nt_status (status);
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 043ccdb99..f86a93825 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1071,6 +1071,27 @@ out:
/* Stop transaction if we started one. */
if (trans)
stop_transaction (status, old_trans, trans);
+
+ /* Check for existence of remote dirs after trying to delete them.
+ Two reasons:
+ - Sometimes SMB indicates failure when it really succeeds.
+ - Removing a directory on a Samba drive using an old Samba version
+ sometimes doesn't return an error, if the directory can't be removed
+ because it's not empty. */
+ if (pc.isdir () && pc.isremote ())
+{
+ FILE_BASIC_INFORMATION fbi;
+ NTSTATUS q_status;
+
+ pc.get_object_attr (attr, sec_none_nih);
+ q_status = NtQueryAttributesFile (&attr, &fbi);
+ if (!NT_SUCCESS (status) && q_status == STATUS_OBJECT_NAME_NOT_FOUND)
+status = STATUS_SUCCESS;
+ else if (pc.fs_is_samba ()
+ && NT_SUCCESS (status) && NT_SUCCESS (q_status))
+status = STATUS_DIRECTORY_NOT_EMPTY;
+}
+
syscall_printf ("%S, return status = %y", pc.get_nt_native_path (), status);
return status;
}
--
2.29.2
[PATCH 04/11] syscalls.cc: Use EISDIR
This is the non-POSIX value returned by Linux since 2.1.132.
---
winsup/cygwin/syscalls.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 227d1a911..043ccdb99 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1118,7 +1118,7 @@ unlink (const char *ourname)
else if (win32_name.isdir ())
{
debug_printf ("unlinking a directory");
- set_errno (EPERM);
+ set_errno (EISDIR);
goto done;
}
--
2.29.2
[PATCH 03/11] syscalls.cc: Fix num_links
NtQueryInformationFile on fh_ro needs FILE_READ_ATTRIBUTES
to succeed.
---
winsup/cygwin/syscalls.cc | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 0e89b4f44..227d1a911 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -767,8 +767,9 @@ _unlink_nt (path_conv &pc, bool shareable)
if ((pc.fs_flags () & FILE_SUPPORTS_TRANSACTIONS))
start_transaction (old_trans, trans);
retry_open:
- status = NtOpenFile (&fh_ro, FILE_WRITE_ATTRIBUTES, &attr, &io,
- FILE_SHARE_VALID_FLAGS, flags);
+ status = NtOpenFile (&fh_ro,
+ FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES,
+ &attr, &io, FILE_SHARE_VALID_FLAGS, flags);
if (NT_SUCCESS (status))
{
debug_printf ("Opening %S for removing R/O succeeded",
--
2.29.2
[PATCH 00/11] Improve rm speed
Hi, I have been working on speeding up rm. The idea is to save on kernel calls. While kernel calls are fast, not doing them is still a lot faster. I do think there is more to gain, but before I proceed it's best to first see if this is something you're willing to commit. I guess the first five patches are trivial, I would really like some feedback on the last six. Also, I'd like to state: I provide my patches to the Cygwin sources under the 2-clause BSD license Thank you, Ben Wijen (11): syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first syscalls.cc: Deduplicate _remove_r syscalls.cc: Fix num_links syscalls.cc: Use EISDIR Cygwin: Move post-dir unlink check cxx.cc: Fix dynamic initialization for static local variables syscalls.cc: Implement non-path_conv dependent _unlink_nt path.cc: Allow to skip filesystem checks mount.cc: Implement poor-man's cache syscalls.cc: Expose shallow-pathconv unlink_nt dir.cc: Try unlink_nt first winsup/cygwin/Makefile.in | 2 +- winsup/cygwin/cxx.cc| 10 - winsup/cygwin/dir.cc| 6 + winsup/cygwin/fhandler_disk_file.cc | 24 +- winsup/cygwin/forkable.cc | 4 +- winsup/cygwin/mount.cc | 78 -- winsup/cygwin/mount.h | 2 +- winsup/cygwin/ntdll.h | 3 +- winsup/cygwin/path.cc | 5 +- winsup/cygwin/path.h| 2 + winsup/cygwin/syscalls.cc | 366 +++- 11 files changed, 384 insertions(+), 118 deletions(-) -- 2.29.2
[PATCH 02/11] syscalls.cc: Deduplicate _remove_r
The _remove_r code is already in the remove function.
Therefore, just call the remove function and make
sure errno is set correctly in the reent struct.
---
winsup/cygwin/syscalls.cc | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index ce4e9c65c..0e89b4f44 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -1133,18 +1133,15 @@ unlink (const char *ourname)
}
extern "C" int
-_remove_r (struct _reent *, const char *ourname)
+_remove_r (struct _reent *ptr, const char *ourname)
{
- path_conv win32_name (ourname, PC_SYM_NOFOLLOW);
+ int ret;
- if (win32_name.error)
-{
- set_errno (win32_name.error);
- syscall_printf ("%R = remove(%s)",-1, ourname);
- return -1;
-}
+ errno = 0;
+ if ((ret = remove (ourname)) == -1 && errno != 0)
+ptr->_errno = errno;
- return win32_name.isdir () ? rmdir (ourname) : unlink (ourname);
+ return ret;
}
extern "C" int
--
2.29.2
[PATCH 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first
---
winsup/cygwin/ntdll.h | 3 ++-
winsup/cygwin/syscalls.cc | 20
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h
index d4f6aaf45..7eee383dd 100644
--- a/winsup/cygwin/ntdll.h
+++ b/winsup/cygwin/ntdll.h
@@ -497,7 +497,8 @@ enum {
FILE_DISPOSITION_DELETE = 0x01,
FILE_DISPOSITION_POSIX_SEMANTICS = 0x02,
FILE_DISPOSITION_FORCE_IMAGE_SECTION_CHECK = 0x04,
- FILE_DISPOSITION_ON_CLOSE= 0x08
+ FILE_DISPOSITION_ON_CLOSE= 0x08,
+ FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE = 0x10,
};
enum
diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 525efecf3..ce4e9c65c 100644
--- a/winsup/cygwin/syscalls.cc
+++ b/winsup/cygwin/syscalls.cc
@@ -709,11 +709,23 @@ _unlink_nt (path_conv &pc, bool shareable)
flags);
if (!NT_SUCCESS (status))
goto out;
- /* Why didn't the devs add a FILE_DELETE_IGNORE_READONLY_ATTRIBUTE
-flag just like they did with FILE_LINK_IGNORE_READONLY_ATTRIBUTE
-and FILE_LINK_IGNORE_READONLY_ATTRIBUTE???
+ /* Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first
+ it was added with Redstone 5 (Win10 18_09) (as were POSIX rename
semantics)
+ If it fails: fall-back to usual trickery */
+ if (wincap.has_posix_rename_semantics ())
+{
+ fdie.Flags = FILE_DISPOSITION_DELETE |
FILE_DISPOSITION_POSIX_SEMANTICS;
+ fdie.Flags|= FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE;
+ status = NtSetInformationFile (fh, &io, &fdie, sizeof fdie,
+ FileDispositionInformationEx);
+ if (NT_SUCCESS (status))
+{
+ NtClose (fh);
+ goto out;
+}
+}
- POSIX unlink semantics are nice, but they still fail if the file
+ /* POSIX unlink semantics are nice, but they still fail if the file
has the R/O attribute set. Removing the file is very much a safe
bet afterwards, so, no transaction. */
if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
--
2.29.2
Re: [PATCH] mkdir: always check-for-existence
Hi Corinna,
Please see the attachment for my patch.
My MUA indeed replaced the tabs with spaces.
I did notice that the indentation was mixed tabs and spaces,
but as stated on the website I have kept the surrounding indentation.
Ben...
On 04-06-2019 09:41, Corinna Vinschen wrote:
Hi Ben,
On Jun 3 22:07, Ben wrote:
When creating a directory which already exists, NtCreateFile will correctly
return 'STATUS_OBJECT_NAME_COLLISION'.
However when creating a directory and all its parents a normal use would
be to start with mkdir(‘/cygdrive/c’) which translates to ‘C:\’ for which
it'll
instead return ‘STATUS_ACCESS_DENIED’.
So we better check for existence prior to calling NtCreateFile.
---
winsup/cygwin/dir.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index f43eae461..b757851d5 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -331,8 +331,10 @@ mkdir (const char *dir, mode_t mode)
debug_printf ("got %d error from build_fh_name", fh->error ());
set_errno (fh->error ());
}
+ else if (fh->exists ())
+ set_errno (EEXIST);
else if (has_dot_last_component (dir, true))
- set_errno (fh->exists () ? EEXIST : ENOENT);
+ set_errno (ENOENT);
else if (!fh->mkdir (mode))
res = 0;
delete fh;
--
2.21.0
I was just trying to apply your patch but it fails to apply cleanly.
Can you please check your indentation? The `else' lines are indented
more than the lines in between and TABs are missing.
Maybe your MUA breaks the output? If all else fails, you could attach
your patch as plain/text attachement to your mail, usually that's left
alone by the MUA.
Thanks,
Corinna
>From 190b5bc9497a1332ce53afd831debe1ac3e53ffb Mon Sep 17 00:00:00 2001
From: Ben Wijen
Date: Mon, 3 Jun 2019 20:15:50 +0200
Subject: [PATCH] mkdir: always check-for-existence
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
When using NtCreateFile when creating a directory that already exists,
it will correctly return 'STATUS_OBJECT_NAME_COLLISION'.
However using this function to create a directory (and all its parents)
a normal use would be to start with mkdir(â/cygdrive/câ) which translates
to âC:\â for which it'll instead return âSTATUS_ACCESS_DENIEDâ.
---
winsup/cygwin/dir.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index f43eae461..b757851d5 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -331,8 +331,10 @@ mkdir (const char *dir, mode_t mode)
debug_printf ("got %d error from build_fh_name", fh->error ());
set_errno (fh->error ());
}
+ else if (fh->exists ())
+ set_errno (EEXIST);
else if (has_dot_last_component (dir, true))
- set_errno (fh->exists () ? EEXIST : ENOENT);
+ set_errno (ENOENT);
else if (!fh->mkdir (mode))
res = 0;
delete fh;
--
2.21.0
Re: [PATCH] mkdir: always check-for-existence
When creating a directory which already exists, NtCreateFile will correctly
return 'STATUS_OBJECT_NAME_COLLISION'.
However when creating a directory and all its parents a normal use would
be to start with mkdir(‘/cygdrive/c’) which translates to ‘C:\’ for
which it'll
instead return ‘STATUS_ACCESS_DENIED’.
So we better check for existence prior to calling NtCreateFile.
---
winsup/cygwin/dir.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index f43eae461..b757851d5 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -331,8 +331,10 @@ mkdir (const char *dir, mode_t mode)
debug_printf ("got %d error from build_fh_name", fh->error ());
set_errno (fh->error ());
}
+ else if (fh->exists ())
+ set_errno (EEXIST);
else if (has_dot_last_component (dir, true))
- set_errno (fh->exists () ? EEXIST : ENOENT);
+ set_errno (ENOENT);
else if (!fh->mkdir (mode))
res = 0;
delete fh;
--
2.21.0
[PATCH] mkdir: alway check-for-existence
When using either CreateDirectory or NtCreateFile when creating a directory
that already exists, these functions return: ERROR_ALREADY_EXISTS
However when using these function to create a directory (and all its
parents)
a normal use would be to start with mkdir(‘/c’) which translates to ‘C:\’
for which both of these functions return ‘ERROR_ACCESS_DENIED’
We could call NtCreateFile with 'FILE_OPEN_IF' instead of 'FILE_CREATE' but
since that's an internal function we better always check for existence
ourselves.
Signed-off-by: Ben Wijen
---
winsup/cygwin/dir.cc | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/dir.cc b/winsup/cygwin/dir.cc
index f43eae461..b757851d5 100644
--- a/winsup/cygwin/dir.cc
+++ b/winsup/cygwin/dir.cc
@@ -331,8 +331,10 @@ mkdir (const char *dir, mode_t mode)
debug_printf ("got %d error from build_fh_name", fh->error ());
set_errno (fh->error ());
}
+ else if (fh->exists ())
+ set_errno (EEXIST);
else if (has_dot_last_component (dir, true))
- set_errno (fh->exists () ? EEXIST : ENOENT);
+ set_errno (ENOENT);
else if (!fh->mkdir (mode))
res = 0;
delete fh;
--
2.21.0
