[PATCH 00/11] Improve rm speed

[PATCH 00/11] Improve rm speed

[Ben Wijen]

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 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first

[Ben Wijen]

---
 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 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first

[Corinna Vinschen]

Hi Ben,

On Jan 15 14:45, Ben Wijen wrote:
> ---
>  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,

How on earth did I miss this flag?


Thanks,
Corinna

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

[Corinna Vinschen]

Hi Ben,

after venting that I missed this flag, back to the patch itself:

On Jan 15 14:45, Ben Wijen wrote:
> ---
>  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;
> +            }
> +        }

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.


Thanks,
Corinna

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

[Ben]

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 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first

[Corinna Vinschen]

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.


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

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

> 
> 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 :)


Corinna

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

[Ben]

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 01/11] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE first

[Corinna Vinschen]

On Jan 18 15:30, Ben wrote:
> 
> 
> 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.

I'm sure, but that code path is called on non-remote ntfs only anyway.


Corinna

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

[Ben]

> 
> I'm sure, but that code path is called on non-remote ntfs only anyway.
> 

Ofcourse, I was thinking about the new _unlink_nt...

[PATCH 02/11] syscalls.cc: Deduplicate _remove_r

[Ben Wijen]

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

Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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;
>  }

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?


Thanks,
Corinna

Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r

[Ben]

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 02/11] syscalls.cc: Deduplicate _remove_r

[Corinna Vinschen]

On Jan 18 13:40, Ben wrote:
> 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.

What about this instead?  It should be better optimizable:

diff --git a/winsup/cygwin/syscalls.cc b/winsup/cygwin/syscalls.cc
index 4742c665339c..2d8acb4c1052 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 (_GLOBAL_REENT, ourname);
 }
 
 extern "C" pid_t



Corinna

Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r

[Ben]

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

[Corinna Vinschen]

On Jan 18 14:51, Ben wrote:
> 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

This is redundant:

  errno == (*__errno ()) == _REENT->_errno == __getreent ()->errno

So ptr->_errno is already set.

> * _GLOBAL_REENT isn't threadlocal, what about __getreent()

Yeah, that makes sense.  Just use the _REENT macro instead.

Care to send the resulting patch?


Thanks,
Corinna

Re: [PATCH 02/11] syscalls.cc: Deduplicate _remove_r

[Ben]

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

[PATCH 03/11] syscalls.cc: Fix num_links

[Ben Wijen]

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

Re: [PATCH 03/11] syscalls.cc: Fix num_links

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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

Oh, right!  Pushed.


Thanks,
Corinna

[PATCH 04/11] syscalls.cc: Use EISDIR

[Ben Wijen]

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

Re: [PATCH 04/11] syscalls.cc: Use EISDIR

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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

Pushed.


Thanks,
Corinna

[PATCH 05/11] Cygwin: Move post-dir unlink check

[Ben Wijen]

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

Re: [PATCH 05/11] Cygwin: Move post-dir unlink check

[Corinna Vinschen]

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.


Corinna

Re: [PATCH 05/11] Cygwin: Move post-dir unlink check

[Ben]

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 05/11] Cygwin: Move post-dir unlink check

[Corinna Vinschen]

On Jan 18 15:31, Ben wrote:
> 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.

Sure, looks good.  You don't have to cramp the text into the first 40
cols, 80 is fine.


Thanks,
Corinna

[PATCH 06/11] cxx.cc: Fix dynamic initialization for static local variables

[Ben Wijen]

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

Re: [PATCH 06/11] cxx.cc: Fix dynamic initialization for static local variables

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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

Pushed.


Thanks,
Corinna

[PATCH 07/11] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Ben Wijen]

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) || // 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
+      //Reopen with all sharing flags, will set delete flag ourselves.
+      access |= FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES;
+      flags &= ~FILE_DELETE_ON_CLOSE;
+      fstatus = NtOpenFile (&fh, access, &attr, &io, FILE_SHARE_VALID_FLAGS, flags);
+      debug_printf("NtOpenFile %S: %y", ntpath, 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.
+              FILE_DISPOSITION_INFORMATION_EX fdie =
+                { FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS
+                    | FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE };
+              istatus = NtSetInformationFile (fh, &io, &fdie, sizeof fdie,
+                                              FileDispositionInformationEx);
+              debug_printf("NtSetInformation %S: %y", ntpath, istatus);
+
+              if(istatus == STATUS_INVALID_PARAMETER)
+                has_posix_unlink_semantics_with_ignore_readonly = false;
+            }
+
+          if (!has_posix_unlink_semantics_with_ignore_readonly)
+            {
+              //Step 4
+              //Check if we must clear the READONLY flag
+              FILE_BASIC_INFORMATION qfbi = { 0 };
+              istatus = NtQueryInformationFile (fh, &io, &qfbi, sizeof qfbi,
+                                                FileBasicInformation);
+              debug_printf("NtQueryFileBasicInformation %S: %y", ntpath,
+                           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", ntpath,
+                                   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",
+                              ntpath, istatus);
+
+                          if(istatus == STATUS_INVALID_PARAMETER)
+                            has_posix_unlink_semantics = false;
+                        }
+
+                      if(!has_posix_unlink_semantics)
+                        {
+                          //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 is the file is in use.
+                          FILE_DISPOSITION_INFORMATION fdi = { TRUE };
+                          istatus = NtSetInformationFile (
+                              fh, &io, &fdi, sizeof fdi,
+                              FileDispositionInformation);
+                          debug_printf("NtSetFileDispositionInformation %S: %y",
+                                       ntpath, istatus);
+                        }
+
+                      if (qfbi.FileAttributes & FILE_ATTRIBUTE_READONLY)
+                        {
+                          //Step 7
+                          //Always mark the file as READONLY (as it was before) before closing the handle
+                          //* In case of failure: This file has correct attributes
+                          //* In case of hardlinks: The hardlinks have the correct attributes
+                          //* In case of success: This file is gone
+                          NTSTATUS tstatus = NtSetInformationFile (
+                              fh, &io, &qfbi, sizeof qfbi,
+                              FileBasicInformation);
+                          debug_printf("NtSetFileBasicInformation %S: %y",
+                                       ntpath, tstatus);
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+  if (NT_SUCCESS(fstatus))
+    {
+      fstatus = NtClose (fh);
+
+      if (!NT_SUCCESS(istatus))
+        {
+          fstatus = istatus;
+        }
+    }
+
+  if (!(eflags & FILE_NON_DIRECTORY_FILE))
+    {
+      fs_info fs;
+      bool is_remote = true;
+      bool is_samba = false;
+      if(fs.update (ntpath, NULL, true)) {
+        is_remote = fs.is_remote_drive();
+        is_samba = fs.is_samba();
+      }
+
+      /* Check for existence of remote dirs after trying to delete them.
+         See unlink_ntpc */
+      if(is_remote) {
+        FILE_BASIC_INFORMATION fbi;
+        istatus = NtQueryAttributesFile (&attr, &fbi);
+        if (!NT_SUCCESS(fstatus) && istatus == STATUS_OBJECT_NAME_NOT_FOUND)
+          fstatus = STATUS_SUCCESS;
+        else if (NT_SUCCESS (fstatus) && NT_SUCCESS(istatus))
+          fstatus = is_samba ? STATUS_DIRECTORY_NOT_EMPTY : STATUS_CANNOT_DELETE;
+      }
+    }
+
+  if (fstatus == STATUS_FILE_DELETED)
+    {
+      fstatus = STATUS_SUCCESS;
+    }
+
+  syscall_printf("%S, return status = %y", ntpath, fstatus);
+  return fstatus;
+}
+
+static NTSTATUS
+_unlink_ntpc (path_conv &pc, bool shareable)
 {
   NTSTATUS status;
   HANDLE fh, fh_ro = NULL;
@@ -685,7 +865,7 @@ _unlink_nt (path_conv &pc, bool shareable)
   int reopened = 0;
   bin_status bin_stat = dont_move;
 
-  syscall_printf ("Trying to delete %S, isdir = %d",
+  syscall_printf ("Fallback delete %S, isdir = %d",
 		  pc.get_nt_native_path (), pc.isdir ());
 
   /* Add the reparse point flag to known reparse points, otherwise we remove
@@ -1096,16 +1276,45 @@ out:
   return status;
 }
 
+static bool
+check_unlink_status(NTSTATUS status)
+{
+  //Return true when we don't want to call _unlink_ntpc
+  return NT_SUCCESS (status) ||
+      status == STATUS_FILE_IS_A_DIRECTORY ||
+      status == STATUS_DIRECTORY_NOT_EMPTY ||
+      status == STATUS_NOT_A_DIRECTORY ||
+      status == STATUS_OBJECT_NAME_NOT_FOUND ||
+      status == STATUS_OBJECT_PATH_INVALID ||
+      status == STATUS_OBJECT_PATH_NOT_FOUND;
+}
+
+static NTSTATUS
+_unlink_ntpc_ (path_conv& pc, bool shareable)
+{
+  PUNICODE_STRING ntpath = pc.get_nt_native_path ();
+  ULONG eflags = pc.isdir () ? FILE_DIRECTORY_FILE : FILE_NON_DIRECTORY_FILE;
+  ULONG oattr  = pc.objcaseinsensitive ();
+
+  NTSTATUS status = _unlink_nt (ntpath, eflags, oattr);
+  if(!check_unlink_status (status))
+  {
+    status = _unlink_ntpc (pc, shareable);
+  }
+
+  return status;
+}
+
 NTSTATUS
-unlink_nt (path_conv &pc)
+unlink_ntpc (path_conv &pc)
 {
-  return _unlink_nt (pc, false);
+  return _unlink_ntpc_ (pc, false);
 }
 
 NTSTATUS
-unlink_nt_shareable (path_conv &pc)
+unlink_ntpc_shareable (path_conv &pc)
 {
-  return _unlink_nt (pc, true);
+  return _unlink_ntpc_ (pc, true);
 }
 
 extern "C" int
@@ -1143,7 +1352,7 @@ unlink (const char *ourname)
       goto done;
     }
 
-  status = unlink_nt (win32_name);
+  status = unlink_ntpc (win32_name);
   if (NT_SUCCESS (status))
     res = 0;
   else
@@ -2658,10 +2867,10 @@ rename2 (const char *oldpath, const char *newpath, unsigned int at2flags)
 	 ReplaceIfExists is set to TRUE and the existing dir is empty.  So
 	 we have to remove the destination dir first.  This also covers the
 	 case that the destination directory is not empty.  In that case,
-	 unlink_nt returns with STATUS_DIRECTORY_NOT_EMPTY. */
+	 unlink_ntpc returns with STATUS_DIRECTORY_NOT_EMPTY. */
       if (dstpc->isdir ())
 	{
-	  status = unlink_nt (*dstpc);
+	  status = unlink_ntpc (*dstpc);
 	  if (!NT_SUCCESS (status))
 	    {
 	      __seterrno_from_nt_status (status);
@@ -2802,13 +3011,13 @@ skip_pre_W10_checks:
 					? FILE_OPEN_REPARSE_POINT : 0));
 	      if (NT_SUCCESS (status))
 		{
-		  status = unlink_nt (*dstpc);
+		  status = unlink_ntpc (*dstpc);
 		  if (NT_SUCCESS (status))
 		    break;
 		}
 	      if (!NT_TRANSACTIONAL_ERROR (status) || !trans)
 		break;
-	      /* If NtOpenFile or unlink_nt fail due to transactional problems,
+	      /* If NtOpenFile or unlink_ntpc fail due to transactional problems,
 		 stop transaction and retry without. */
 	      NtClose (fh);
 	      stop_transaction (status, old_trans, trans);
@@ -2822,7 +3031,7 @@ skip_pre_W10_checks:
       if (NT_SUCCESS (status))
 	{
 	  if (removepc)
-	    unlink_nt (*removepc);
+	    unlink_ntpc (*removepc);
 	  res = 0;
 	}
       else
-- 
2.29.2

Re: [PATCH 07/11] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Ben]

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 08/11] path.cc: Allow to skip filesystem checks

[Ben Wijen]

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

Re: [PATCH 08/11] path.cc: Allow to skip filesystem checks

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> When file attributes are of no concern,
> there is no point to query them.

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.

> ---
>  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);
> +	    }

Please follow the indentation 4 lines above, and please don't add curly
braces for single line blocks.


Thanks,
Corinna

Re: [PATCH 08/11] path.cc: Allow to skip filesystem checks

[Ben]

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 08/11] path.cc: Allow to skip filesystem checks

[Corinna Vinschen]

On Jan 18 18:15, Ben wrote:
> 
> 
> 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.

That's not necessary, but a log msg hint along the lines of "in
preparation of <what follow up patch is doing>" would help.

However, just to be clear here.  While I really appreciate your efforts,
I'm not sure yet if we really *can* skip the filesystem checks.  This is
dangerous but often perpetrated territory.  Trying to work around
certain checks almost always resulted in other problems in the past,
like POSIX/Linux incompatibilities or forgetting to handle Windows
quirks.  A certain callousness and high frustration barrier are
required.


Thanks,
Corinna

[PATCH 09/11] mount.cc: Implement poor-man's cache

[Ben Wijen]

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] ? fsn : "unknown"; }
 
-  bool __reg3 update (PUNICODE_STRING, HANDLE);
+  bool __reg3 update (PUNICODE_STRING, HANDLE, bool=false);
   bool inited () const { return !!status.flags; }
 };
 
diff --git a/winsup/cygwin/path.cc b/winsup/cygwin/path.cc
index f00707e86..9e803f986 100644
--- a/winsup/cygwin/path.cc
+++ b/winsup/cygwin/path.cc
@@ -1179,7 +1179,7 @@ path_conv::check (const char *src, unsigned opt,
 	{
 	  /* If FS hasn't been checked already in symlink_info::check,
 	     do so now. */
-	  if (fs.inited ()|| fs.update (get_nt_native_path (), NULL))
+	  if (fs.inited ()|| fs.update (get_nt_native_path (), NULL, opt & PC_FS_USE_PREFIX_HASH))
 	    {
 	      /* Incoming DOS paths are treated like DOS paths in native
 		 Windows applications.  No ACLs, just default settings. */
diff --git a/winsup/cygwin/path.h b/winsup/cygwin/path.h
index 56855e1c9..b9264811e 100644
--- a/winsup/cygwin/path.h
+++ b/winsup/cygwin/path.h
@@ -60,6 +60,7 @@ enum pathconv_arg
   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_FS_USE_PREFIX_HASH	 = _BIT (16),	/* in fs_info search by prefix hash */
   PC_DONT_USE		 = _BIT (31)	/* conversion to signed happens. */
 };
 
-- 
2.29.2

Re: [PATCH 09/11] mount.cc: Implement poor-man's cache

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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);
> +}
> +

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.

> @@ -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);

Please note that we did this *only* for border case FSes returning a VSN
of 0.  This was sufficient for these cases, but not in a a general sense.


Corinna

Re: [PATCH 09/11] mount.cc: Implement poor-man's cache

[Ben]

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 09/11] mount.cc: Implement poor-man's cache

[Corinna Vinschen]

On Feb  3 12:38, Ben wrote:
> 
> 
> 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

I wasn't talking about Cygwin mount points, but rather about Windows
mount points.  Since Windows 2000 a partition can be mounted into a
directory of another partition.  Only drive C: (ignoring non-harddisks)
has to be mounted with a drive letter, all others can be mounted just as
on Unix.

But, yeah, Cygwin also supports bind mounts.  Here's an example
from my /etc/fstab.d user mount file:

  //remote/cygwin-src /cygwin nfs binary 0 0
  /cygwin/pub /home/pub nfs bind 0 0


Corinna

[PATCH 10/11] syscalls.cc: Expose shallow-pathconv unlink_nt

[Ben Wijen]

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 11/11] dir.cc: Try unlink_nt first

[Ben Wijen]

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

Re: [PATCH 11/11] dir.cc: Try unlink_nt first

[Corinna Vinschen]

On Jan 15 14:45, Ben Wijen wrote:
> 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))) {
          ^^         ^^
          spaces

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.


Corinna

Re: [PATCH 11/11] dir.cc: Try unlink_nt first

[Ben]

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 11/11] dir.cc: Try unlink_nt first

[Ben]

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 11/11] dir.cc: Try unlink_nt first

[Corinna Vinschen]

On Jan 18 18:07, Ben wrote:
> 
> 
> 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.

No, isdev_dev in rmdir returns ENOTEMPTY because /dev is a merge between
a virtual and a real directory.  You can write into that directory by
adding new entries, like the symlinks for stdin/stdout, etc., but
of course it's a non-empty dir.  It's a kind of stop-gap measure so
/dev doesn't get removed accidentally.

In retrospect, checking against isdev_dev is a bit unclean here.  It
would be cleaner to add a method fhandler_dev::rmdir to override
the rmdir method of the underlying fhandler_disk_file class and handle
this in the fhandler class as desired.

I pushed a matching patch.


Corinna

[PATCH v2 0/8] Improve rm speed

[Ben Wijen]

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

[PATCH v2 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

[Ben Wijen]

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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -208,6 +213,7 @@ wincaps  wincap_10_1607 __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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -242,6 +248,7 @@ wincaps wincap_10_1703 __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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -276,6 +283,7 @@ wincaps wincap_10_1709 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    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,
@@ -310,6 +318,7 @@ wincaps wincap_10_1803 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:false,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -344,6 +353,7 @@ wincaps wincap_10_1809 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:true,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:true,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -378,6 +388,7 @@ wincaps wincap_10_1903 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:true,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:true,
     no_msv1_0_s4u_logon_in_wow64:false,
diff --git a/winsup/cygwin/wincap.h b/winsup/cygwin/wincap.h
index 2f4191aa1..687e51843 100644
--- a/winsup/cygwin/wincap.h
+++ b/winsup/cygwin/wincap.h
@@ -16,33 +16,34 @@ struct wincaps
   /* The bitfields must be 8 byte aligned on x86_64, otherwise the bitfield
      ops generated by gcc are off by 4 bytes. */
   struct  __attribute__ ((aligned (8))) {
-    unsigned is_server				: 1;
-    unsigned needs_count_in_si_lpres2		: 1;
-    unsigned needs_query_information		: 1;
-    unsigned has_gaa_largeaddress_bug		: 1;
-    unsigned has_broken_alloc_console		: 1;
-    unsigned has_console_logon_sid		: 1;
-    unsigned has_precise_system_time		: 1;
-    unsigned has_microsoft_accounts		: 1;
-    unsigned has_processor_groups		: 1;
-    unsigned has_broken_prefetchvm		: 1;
-    unsigned has_new_pebteb_region		: 1;
-    unsigned has_broken_whoami			: 1;
-    unsigned has_unprivileged_createsymlink	: 1;
-    unsigned has_unbiased_interrupt_time	: 1;
-    unsigned has_precise_interrupt_time		: 1;
-    unsigned has_posix_unlink_semantics		: 1;
-    unsigned has_case_sensitive_dirs		: 1;
-    unsigned has_posix_rename_semantics		: 1;
-    unsigned no_msv1_0_s4u_logon_in_wow64	: 1;
-    unsigned has_con_24bit_colors		: 1;
-    unsigned has_con_broken_csi3j		: 1;
-    unsigned has_con_broken_il_dl		: 1;
-    unsigned has_con_esc_rep			: 1;
-    unsigned has_extended_mem_api		: 1;
-    unsigned has_tcp_fastopen			: 1;
-    unsigned has_linux_tcp_keepalive_sockopts	: 1;
-    unsigned has_tcp_maxrtms			: 1;
+    unsigned is_server						: 1;
+    unsigned needs_count_in_si_lpres2				: 1;
+    unsigned needs_query_information				: 1;
+    unsigned has_gaa_largeaddress_bug				: 1;
+    unsigned has_broken_alloc_console				: 1;
+    unsigned has_console_logon_sid				: 1;
+    unsigned has_precise_system_time				: 1;
+    unsigned has_microsoft_accounts				: 1;
+    unsigned has_processor_groups				: 1;
+    unsigned has_broken_prefetchvm				: 1;
+    unsigned has_new_pebteb_region				: 1;
+    unsigned has_broken_whoami					: 1;
+    unsigned has_unprivileged_createsymlink			: 1;
+    unsigned has_unbiased_interrupt_time			: 1;
+    unsigned has_precise_interrupt_time				: 1;
+    unsigned has_posix_unlink_semantics				: 1;
+    unsigned has_posix_unlink_semantics_with_ignore_readonly	: 1;
+    unsigned has_case_sensitive_dirs				: 1;
+    unsigned has_posix_rename_semantics				: 1;
+    unsigned no_msv1_0_s4u_logon_in_wow64			: 1;
+    unsigned has_con_24bit_colors				: 1;
+    unsigned has_con_broken_csi3j				: 1;
+    unsigned has_con_broken_il_dl				: 1;
+    unsigned has_con_esc_rep					: 1;
+    unsigned has_extended_mem_api				: 1;
+    unsigned has_tcp_fastopen					: 1;
+    unsigned has_linux_tcp_keepalive_sockopts			: 1;
+    unsigned has_tcp_maxrtms					: 1;
   };
 };
 
@@ -98,6 +99,7 @@ public:
   bool	IMPLEMENT (has_unbiased_interrupt_time)
   bool	IMPLEMENT (has_precise_interrupt_time)
   bool	IMPLEMENT (has_posix_unlink_semantics)
+  bool  IMPLEMENT (has_posix_unlink_semantics_with_ignore_readonly)
   bool	IMPLEMENT (has_case_sensitive_dirs)
   bool	IMPLEMENT (has_posix_rename_semantics)
   bool	IMPLEMENT (no_msv1_0_s4u_logon_in_wow64)
-- 
2.30.0

Re: [PATCH v2 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

[Corinna Vinschen]

Hi Ben,

On Jan 20 17:10, Ben Wijen wrote:
> 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);

A few lines above, FILE_WRITE_ATTRIBUTES is added to the
access mask, if the file is R/O.  This, too, depends on
wincap.has_posix_unlink_semantics_with_ignore_readonly().

>        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. */

This comment should contain a short comment "W10 1809+, blah blah",
analogue to the comment in line 698 in terms of 1709+ ("++"?  Oops,
fix typo...).


> -      if (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
> +      if (!wincap.has_posix_unlink_semantics_with_ignore_readonly ()
> +          && (pc.file_attributes () & FILE_ATTRIBUTE_READONLY))

I'd invert the test order here.  On 1809+ systems, the majority of
systems these days, the first test is always true, but it's always
checked, even if the file is not R/O.  First checking for R/O would
reduce the hits on the "with_ignore_readonly" check.

>  	{
>  	  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 ())
          ^^^
          space
> +          fdie.Flags |= FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE;
         ^^^^
         indentation 2, not 4.

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

Same here.

Actually, on second thought, what about introducing another bool at the
start of the posix handling, along the lines of

   const bool needs_ro_handling =
     (pc.file_attributes () & FILE_ATTRIBUTE_READONLY)
     && !wincap.has_posix_unlink_semantics_with_ignore_readonly ();

and then check for needs_ro_handling throughout?


Thanks,
Corinna

[PATCH v3 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

[Ben Wijen]

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.GD810271@calimero.vinschen.de>
References: <20210122105201.GD810271@calimero.vinschen.de>
From: Ben Wijen <ben@wijen.net>
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_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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -208,6 +213,7 @@ wincaps  wincap_10_1607 __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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -242,6 +248,7 @@ wincaps wincap_10_1703 __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_sensitive_dirs:false,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -276,6 +283,7 @@ wincaps wincap_10_1709 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    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,
@@ -310,6 +318,7 @@ wincaps wincap_10_1803 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:false,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:false,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -344,6 +353,7 @@ wincaps wincap_10_1809 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:true,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:true,
     no_msv1_0_s4u_logon_in_wow64:false,
@@ -378,6 +388,7 @@ wincaps wincap_10_1903 __attribute__((section (".cygwin_dll_common"), shared)) =
     has_unbiased_interrupt_time:true,
     has_precise_interrupt_time:true,
     has_posix_unlink_semantics:true,
+    has_posix_unlink_semantics_with_ignore_readonly:true,
     has_case_sensitive_dirs:true,
     has_posix_rename_semantics:true,
     no_msv1_0_s4u_logon_in_wow64:false,
diff --git a/winsup/cygwin/wincap.h b/winsup/cygwin/wincap.h
index 2f4191aa1..687e51843 100644
--- a/winsup/cygwin/wincap.h
+++ b/winsup/cygwin/wincap.h
@@ -16,33 +16,34 @@ struct wincaps
   /* The bitfields must be 8 byte aligned on x86_64, otherwise the bitfield
      ops generated by gcc are off by 4 bytes. */
   struct  __attribute__ ((aligned (8))) {
-    unsigned is_server				: 1;
-    unsigned needs_count_in_si_lpres2		: 1;
-    unsigned needs_query_information		: 1;
-    unsigned has_gaa_largeaddress_bug		: 1;
-    unsigned has_broken_alloc_console		: 1;
-    unsigned has_console_logon_sid		: 1;
-    unsigned has_precise_system_time		: 1;
-    unsigned has_microsoft_accounts		: 1;
-    unsigned has_processor_groups		: 1;
-    unsigned has_broken_prefetchvm		: 1;
-    unsigned has_new_pebteb_region		: 1;
-    unsigned has_broken_whoami			: 1;
-    unsigned has_unprivileged_createsymlink	: 1;
-    unsigned has_unbiased_interrupt_time	: 1;
-    unsigned has_precise_interrupt_time		: 1;
-    unsigned has_posix_unlink_semantics		: 1;
-    unsigned has_case_sensitive_dirs		: 1;
-    unsigned has_posix_rename_semantics		: 1;
-    unsigned no_msv1_0_s4u_logon_in_wow64	: 1;
-    unsigned has_con_24bit_colors		: 1;
-    unsigned has_con_broken_csi3j		: 1;
-    unsigned has_con_broken_il_dl		: 1;
-    unsigned has_con_esc_rep			: 1;
-    unsigned has_extended_mem_api		: 1;
-    unsigned has_tcp_fastopen			: 1;
-    unsigned has_linux_tcp_keepalive_sockopts	: 1;
-    unsigned has_tcp_maxrtms			: 1;
+    unsigned is_server						: 1;
+    unsigned needs_count_in_si_lpres2				: 1;
+    unsigned needs_query_information				: 1;
+    unsigned has_gaa_largeaddress_bug				: 1;
+    unsigned has_broken_alloc_console				: 1;
+    unsigned has_console_logon_sid				: 1;
+    unsigned has_precise_system_time				: 1;
+    unsigned has_microsoft_accounts				: 1;
+    unsigned has_processor_groups				: 1;
+    unsigned has_broken_prefetchvm				: 1;
+    unsigned has_new_pebteb_region				: 1;
+    unsigned has_broken_whoami					: 1;
+    unsigned has_unprivileged_createsymlink			: 1;
+    unsigned has_unbiased_interrupt_time			: 1;
+    unsigned has_precise_interrupt_time				: 1;
+    unsigned has_posix_unlink_semantics				: 1;
+    unsigned has_posix_unlink_semantics_with_ignore_readonly	: 1;
+    unsigned has_case_sensitive_dirs				: 1;
+    unsigned has_posix_rename_semantics				: 1;
+    unsigned no_msv1_0_s4u_logon_in_wow64			: 1;
+    unsigned has_con_24bit_colors				: 1;
+    unsigned has_con_broken_csi3j				: 1;
+    unsigned has_con_broken_il_dl				: 1;
+    unsigned has_con_esc_rep					: 1;
+    unsigned has_extended_mem_api				: 1;
+    unsigned has_tcp_fastopen					: 1;
+    unsigned has_linux_tcp_keepalive_sockopts			: 1;
+    unsigned has_tcp_maxrtms					: 1;
   };
 };
 
@@ -98,6 +99,7 @@ public:
   bool	IMPLEMENT (has_unbiased_interrupt_time)
   bool	IMPLEMENT (has_precise_interrupt_time)
   bool	IMPLEMENT (has_posix_unlink_semantics)
+  bool	IMPLEMENT (has_posix_unlink_semantics_with_ignore_readonly)
   bool	IMPLEMENT (has_case_sensitive_dirs)
   bool	IMPLEMENT (has_posix_rename_semantics)
   bool	IMPLEMENT (no_msv1_0_s4u_logon_in_wow64)
-- 
2.30.0

Re: [PATCH v3 1/8] syscalls.cc: unlink_nt: Try FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE

[Corinna Vinschen]

Hi Ben,

On Jan 22 16:47, Ben Wijen wrote:
> I think we don't need an extra flag as we can utilize: access & FILE_WRITE_ATTRIBUTES
> What do you think?

Good idea.  Pushed.


Corinna

[PATCH v2 2/8] syscalls.cc: Deduplicate remove

[Ben Wijen]

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

Re: [PATCH v2 2/8] syscalls.cc: Deduplicate remove

[Corinna Vinschen]

On Jan 20 17:10, Ben Wijen wrote:
> 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);
                    ^^^
		    space


Corinna

[PATCH v3 2/8] syscalls.cc: Deduplicate remove

[Ben Wijen]

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

Re: [PATCH v3 2/8] syscalls.cc: Deduplicate remove

[Corinna Vinschen]

On Jan 22 16:47, Ben Wijen wrote:
> 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(-)

Pushed.


Thanks,
Corinna

[PATCH v2 3/8] Cygwin: Move post-dir unlink check

[Ben Wijen]

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

Re: [PATCH v2 3/8] Cygwin: Move post-dir unlink check

[Corinna Vinschen]

On Jan 20 17:10, Ben Wijen wrote:
> 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(-)

Pushed.


Thanks,
Corinna

[PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Ben Wijen]

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)    // The file is 'readonly'
+    {
+      //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 (&fh, access, attr, &io, 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.
+              FILE_DISPOSITION_INFORMATION_EX fdie =
+                { FILE_DISPOSITION_DELETE | FILE_DISPOSITION_POSIX_SEMANTICS
+                    | FILE_DISPOSITION_IGNORE_READONLY_ATTRIBUTE };
+              istatus = NtSetInformationFile (fh, &io, &fdie, 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, &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.
+                          FILE_DISPOSITION_INFORMATION fdi = { TRUE };
+                          istatus = NtSetInformationFile (
+                              fh, &io, &fdi, sizeof fdi,
+                              FileDispositionInformation);
+                          debug_printf ("NtSetFileDispositionInformation %S: %y",
+                                        attr->ObjectName, istatus);
+                        }
+
+                      if (qfbi.FileAttributes & FILE_ATTRIBUTE_READONLY)
+                        {
+                          //Step 7
+                          //Always mark the file as READONLY (as it was before) before closing the handle
+                          //* In case of failure: This file has correct attributes
+                          //* In case of hardlinks: The hardlinks have the correct attributes
+                          //* In case of success: This file is gone
+                          NTSTATUS tstatus = NtSetInformationFile (
+                              fh, &io, &qfbi, sizeof qfbi,
+                              FileBasicInformation);
+                          debug_printf ("NtSetFileBasicInformation %S: %y",
+                                        attr->ObjectName, tstatus);
+                        }
+                    }
+                }
+            }
+        }
+    }
+
+  if (NT_SUCCESS (fstatus))
+    {
+      fstatus = NtClose (fh);
+
+      if (!NT_SUCCESS (istatus))
+        fstatus = istatus;
+    }
+
+  if (fstatus == STATUS_FILE_DELETED)
+    fstatus = STATUS_SUCCESS;
+
+  syscall_printf ("%S, return status = %y", attr->ObjectName, fstatus);
+  return fstatus;
+}
+
+static NTSTATUS
+_unlink_ntpc (path_conv &pc, bool shareable)
 {
   NTSTATUS status;
   HANDLE fh, fh_ro = NULL;
@@ -709,7 +859,7 @@ _unlink_nt (path_conv &pc, bool shareable)
   int reopened = 0;
   bin_status bin_stat = dont_move;
 
-  syscall_printf ("Trying to delete %S, isdir = %d",
+  syscall_printf ("Fallback delete %S, isdir = %d",
 		  pc.get_nt_native_path (), pc.isdir ());
 
   /* Add the reparse point flag to known reparse points, otherwise we remove
@@ -1091,16 +1241,47 @@ out:
   return status;
 }
 
+static bool
+check_unlink_status(NTSTATUS status)
+{
+  //Return true when we don't want to call _unlink_ntpc
+  return NT_SUCCESS (status)
+      || status == STATUS_FILE_IS_A_DIRECTORY
+      || status == STATUS_DIRECTORY_NOT_EMPTY
+      || status == STATUS_NOT_A_DIRECTORY
+      || status == STATUS_OBJECT_NAME_NOT_FOUND
+      || status == STATUS_OBJECT_PATH_INVALID
+      || status == STATUS_OBJECT_PATH_NOT_FOUND;
+}
+
+static NTSTATUS
+_unlink_ntpc_ (path_conv& pc, bool shareable)
+{
+  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);
+
+  if (pc.isdir ())
+    status = _unlink_nt_post_dir_check (status, &attr, pc);
+
+  if(!check_unlink_status (status))
+    status = _unlink_ntpc (pc, shareable);
+
+  return status;
+}
+
 NTSTATUS
-unlink_nt (path_conv &pc)
+unlink_ntpc (path_conv &pc)
 {
-  return _unlink_nt (pc, false);
+  return _unlink_ntpc_ (pc, false);
 }
 
 NTSTATUS
-unlink_nt_shareable (path_conv &pc)
+unlink_ntpc_shareable (path_conv &pc)
 {
-  return _unlink_nt (pc, true);
+  return _unlink_ntpc_ (pc, true);
 }
 
 extern "C" int
@@ -1138,7 +1319,7 @@ unlink (const char *ourname)
       goto done;
     }
 
-  status = unlink_nt (win32_name);
+  status = unlink_ntpc (win32_name);
   if (NT_SUCCESS (status))
     res = 0;
   else
@@ -2647,10 +2828,10 @@ rename2 (const char *oldpath, const char *newpath, unsigned int at2flags)
 	 ReplaceIfExists is set to TRUE and the existing dir is empty.  So
 	 we have to remove the destination dir first.  This also covers the
 	 case that the destination directory is not empty.  In that case,
-	 unlink_nt returns with STATUS_DIRECTORY_NOT_EMPTY. */
+	 unlink_ntpc returns with STATUS_DIRECTORY_NOT_EMPTY. */
       if (dstpc->isdir ())
 	{
-	  status = unlink_nt (*dstpc);
+	  status = unlink_ntpc (*dstpc);
 	  if (!NT_SUCCESS (status))
 	    {
 	      __seterrno_from_nt_status (status);
@@ -2791,13 +2972,13 @@ skip_pre_W10_checks:
 					? FILE_OPEN_REPARSE_POINT : 0));
 	      if (NT_SUCCESS (status))
 		{
-		  status = unlink_nt (*dstpc);
+		  status = unlink_ntpc (*dstpc);
 		  if (NT_SUCCESS (status))
 		    break;
 		}
 	      if (!NT_TRANSACTIONAL_ERROR (status) || !trans)
 		break;
-	      /* If NtOpenFile or unlink_nt fail due to transactional problems,
+	      /* If NtOpenFile or unlink_ntpc fail due to transactional problems,
 		 stop transaction and retry without. */
 	      NtClose (fh);
 	      stop_transaction (status, old_trans, trans);
@@ -2811,7 +2992,7 @@ skip_pre_W10_checks:
       if (NT_SUCCESS (status))
 	{
 	  if (removepc)
-	    unlink_nt (*removepc);
+	    unlink_ntpc (*removepc);
 	  res = 0;
 	}
       else
-- 
2.30.0

Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Corinna Vinschen]

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 &pc);
> +  extern NTSTATUS unlink_ntpc (path_conv &pc);

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 &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 ();

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 (&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)    // 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 (&fh, access, attr, &io, 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, &io, &fdie, 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, &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?

> +                          FILE_DISPOSITION_INFORMATION fdi = { TRUE };
> +                          istatus = NtSetInformationFile (
> +                              fh, &io, &fdi, sizeof fdi,
> +                              FileDispositionInformation);
> +                          debug_printf ("NtSetFileDispositionInformation %S: %y",
> +                                        attr->ObjectName, istatus);
> +                        }
> +[...]
> +static bool
> +check_unlink_status(NTSTATUS status)
> +{
> +  //Return true when we don't want to call _unlink_ntpc
> +  return NT_SUCCESS (status)
> +      || status == STATUS_FILE_IS_A_DIRECTORY
> +      || status == STATUS_DIRECTORY_NOT_EMPTY
> +      || status == STATUS_NOT_A_DIRECTORY
> +      || status == STATUS_OBJECT_NAME_NOT_FOUND
> +      || status == STATUS_OBJECT_PATH_INVALID
> +      || status == STATUS_OBJECT_PATH_NOT_FOUND;
> +}
> +
> +static NTSTATUS
> +_unlink_ntpc_ (path_conv& pc, bool shareable)

The trailing underscore should be replaced with a descriptive postfix.

> +{
> +  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?

> +
> +  if (pc.isdir ())
> +    status = _unlink_nt_post_dir_check (status, &attr, pc);
> +
> +  if(!check_unlink_status (status))
> +    status = _unlink_ntpc (pc, shareable);
> +
> +  return status;
> +}
> +


Corinna

Re: [PATCH v2 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Ben]

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 (&fh, access, attr, &io, 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, &io, &fdie, 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, &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 4/8] syscalls.cc: Implement non-path_conv dependent _unlink_nt

[Corinna Vinschen]

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 (&fh, access, attr, &io, 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 high probability.  By
checking the R/O flag, the transaction is only called in those few
cases.

However, my POV on NTFS transactions got quite a dent after MSFT
deprecated them, and even more so after the transaction fiasko in the
rename2 function (see the NT_TRANSACTIONAL_ERROR case there, oh my).

Having said that, we may drop transactions in the non-POSIX-semantics
unlink case, but given the usually minor number of affected files, I
don't quite see the point.

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

I don't get that.  What has the FILE_ATTRIBUTE_ARCHIVE bit to do with a
file marked for deletion?

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

I'm still far from being convinced this is a good idea.  Please keep in
mind that unlinking originally worked very differently.  I created the
unlink_nt function for the exact same reason you're trying to replace it
today: I was trying to shortcut as much as possible so unlinking (or
better: the bordercase called `rm -r') gets a lot faster.

unlink_nt also was quite a bit different from where it is now.  It was
a lengthy and painful process to hack on all problems which showed up
over time.

Shortcutting path_conv *will* result in unhandled or wrongly handled
cases.

What would *really* help: Speeding up path_conv, and maybe correct its
path handling along the way, i. e., handle the path from left to right
per POSIX instead of from right to left.  The latter was also a
shortcut, which still lives on, unfortunately.


Corinna

[PATCH v2 5/8] path.cc: Allow to skip filesystem checks

[Ben Wijen]

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 6/8] syscalls.cc: Expose shallow-pathconv unlink_nt

[Ben Wijen]

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

Re: [PATCH v2 6/8] syscalls.cc: Expose shallow-pathconv unlink_nt

[Corinna Vinschen]

On Jan 20 17:10, Ben Wijen wrote:
> 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);

Sorry again, but I don't see the advantage of not using the intelligence
already collected in path_conv by neglecting to call the unlink
variation not using them.  It's also unclear to me, why the new code
doesn't try_to_bin right away, rather than stomping ahead and falling
back to the current code for each such file.  In an rm -rf on a file
hirarchy used by other users, you could end up with a much slower
operation.

Wouldn't it make more sense to streamline the existing _unlink_nt?  I
don't claim it's the most streamlined way to delete files, but at least
it has documented workarounds for problems encountered on the way,
already.


Corinna

[PATCH v2 7/8] dir.cc: Try unlink_nt first

[Ben Wijen]

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

Re: [PATCH v2 7/8] dir.cc: Try unlink_nt first

[Corinna Vinschen]

On Jan 20 17:10, Ben Wijen wrote:
> 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;
> +      }

So what about /dev, /proc, etc?

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


Corinna

Re: [PATCH v2 7/8] dir.cc: Try unlink_nt first

[Ben]

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 v2 8/8] fhandler_disk_file.cc: Use path_conv's IndexNumber

[Ben Wijen]

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

Re: [PATCH v2 8/8] fhandler_disk_file.cc: Use path_conv's IndexNumber

[Corinna Vinschen]

On Jan 20 17:10, Ben Wijen wrote:
> path_conv already knows the IndexNumber, so just use it.

Yeah, this looks like a vestige from before the time we switched to
FILE_ALL_INFORMATION.

> 
> 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);

Given that this function doesn't need a handle anymore, PC_KEEP_HANDLE
can go away.

> -  if (pc.isspecial ())
> +  if (pc.isgood_inode (pc.fai ()->InternalInformation.IndexNumber.QuadPart))
> +    ino = pc.fai ()->InternalInformation.IndexNumber.QuadPart;

This ignores the fact that the file could be on an NFS filesystem.
Rather than using pc.fai ()->InternalInformation.IndexNumber.QuadPart,
this call should use pc.get_ino ().


Thanks,
Corinna