diff mbox series

libstdc++: testsuite: fs rename to self may fail

Message ID orilot9pw4.fsf@lxoliva.fsfla.org
State Committed
Headers
Series libstdc++: testsuite: fs rename to self may fail |

Commit Message

Alexandre Oliva June 22, 2022, 6:24 a.m. UTC 
  rtems6's rename() implementation errors with EEXIST when the rename-to
filename exists, even when renaming a file to itself or when renaming
a nonexisting file.  Adjust expectations.

Regstrapped on x86_64-linux-gnu, also tested with a cross to
aarch64-rtems6.  Ok to install?

PS: https://devel.rtems.org/ticket/2169 doesn't seem to suggest plans to
change behavior so as to comply with POSIX.


for  libstdc++-v3/ChangeLog

	* testsuite/27_io/filesystem/operations/rename.cc (test01):
	Accept EEXIST fail on self-rename, on rename of a
	nonexisting file, and on rename to existing file.  Clean up p1
	in case it remains.
	* testsuite/experimental/filesystem/operations/rename.cc
	(test01): Accept EEXIST fail on self-rename, and on rename to
	existing file.  Clean up p1 in case it remains.
---
 .../27_io/filesystem/operations/rename.cc          |   11 ++++++-----
 .../experimental/filesystem/operations/rename.cc   |    9 +++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

Comments

Sebastian Huber June 22, 2022, 6:33 a.m. UTC | #1 
On 22/06/2022 08:24, Alexandre Oliva via Libstdc++ wrote:
> rtems6's rename() implementation errors with EEXIST when the rename-to
> filename exists, even when renaming a file to itself or when renaming
> a nonexisting file.  Adjust expectations.
> 
> Regstrapped on x86_64-linux-gnu, also tested with a cross to
> aarch64-rtems6.  Ok to install?
> 
> PS:https://devel.rtems.org/ticket/2169  doesn't seem to suggest plans to
> change behavior so as to comply with POSIX.

I would not adjust the test case to cope with systems which are not in 
line with POSIX. In the past RTEMS used the GCC tests to check that the 
implementation is in line with other systems. The RTEMS ticket is still 
open. There just needs to be someone who thinks this bug is important 
enough to fix.
Alexandre Oliva June 22, 2022, 7:01 a.m. UTC | #2 
Hello, Sebastian,

On Jun 22, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:

> On 22/06/2022 08:24, Alexandre Oliva via Libstdc++ wrote:
>> rtems6's rename() implementation errors with EEXIST when the rename-to
>> filename exists, even when renaming a file to itself or when renaming
>> a nonexisting file.  Adjust expectations.
>> 
>> Regstrapped on x86_64-linux-gnu, also tested with a cross to
>> aarch64-rtems6.  Ok to install?
>> 
>> PS:https://devel.rtems.org/ticket/2169  doesn't seem to suggest plans to
>> change behavior so as to comply with POSIX.

> I would not adjust the test case to cope with systems which are not in
> line with POSIX.

My understanding is that the libstdc++ testsuite is not meant to test
for POSIX conformance, but for conformance with the C++ language
standards.

C++ inherits rename from C, and C says the behavior is implementation
defined if the new name already exists.

RTEMS is thus comformant with the requirements from C (and thus C++),
and it is therefore reasonable for libstdc++'s testsuite to accept
RTEMS' behavior as such.


That said, because libstdc++ tests are all-or-nothing, perhaps it would
make sense to have a separate test for strict POSIX conformance in
rename, XFAILed on RTEMS targets.  How about that?
Jonathan Wakely June 22, 2022, 10:41 a.m. UTC | #3 
On Wed, 22 Jun 2022 at 08:02, Alexandre Oliva via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> Hello, Sebastian,
>
> On Jun 22, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
>
> > On 22/06/2022 08:24, Alexandre Oliva via Libstdc++ wrote:
> >> rtems6's rename() implementation errors with EEXIST when the rename-to
> >> filename exists, even when renaming a file to itself or when renaming
> >> a nonexisting file.  Adjust expectations.
> >>
> >> Regstrapped on x86_64-linux-gnu, also tested with a cross to
> >> aarch64-rtems6.  Ok to install?
> >>
> >> PS:https://devel.rtems.org/ticket/2169  doesn't seem to suggest plans to
> >> change behavior so as to comply with POSIX.
>
> > I would not adjust the test case to cope with systems which are not in
> > line with POSIX.
>
> My understanding is that the libstdc++ testsuite is not meant to test
> for POSIX conformance, but for conformance with the C++ language
> standards.
>
> C++ inherits rename from C, and C says the behavior is implementation
> defined if the new name already exists.

std::filesystem::rename is explicitly specified in terms of POSIX
rename, not C rename. POSIX says:

"If the old argument and the new argument resolve to either the same
existing directory entry or different directory entries for the same
existing file, rename() shall return successfully and perform no other
action." and "If the link named by the new argument exists, it shall
be removed and old renamed to new."

So I agree with Sebastian, the tests are correct.

Instead, the implementation of std::filesystem::rename should have a
special-case for rtems (and maybe other targets) that implements the
POSIX rename semantics if calling ::rename isn't good enough.


>
> RTEMS is thus comformant with the requirements from C (and thus C++),
> and it is therefore reasonable for libstdc++'s testsuite to accept
> RTEMS' behavior as such.
>
>
> That said, because libstdc++ tests are all-or-nothing, perhaps it would
> make sense to have a separate test for strict POSIX conformance in
> rename, XFAILed on RTEMS targets.  How about that?
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>
>
Alexandre Oliva June 23, 2022, 3:59 a.m. UTC | #4 
On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:

> On Wed, 22 Jun 2022 at 08:02, Alexandre Oliva via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
>> 
>> Hello, Sebastian,
>> 
>> On Jun 22, 2022, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
>> 
>> > On 22/06/2022 08:24, Alexandre Oliva via Libstdc++ wrote:
>> >> rtems6's rename() implementation errors with EEXIST when the rename-to
>> >> filename exists, even when renaming a file to itself or when renaming
>> >> a nonexisting file.  Adjust expectations.
>> >>
>> >> Regstrapped on x86_64-linux-gnu, also tested with a cross to
>> >> aarch64-rtems6.  Ok to install?
>> >>
>> >> PS:https://devel.rtems.org/ticket/2169  doesn't seem to suggest plans to
>> >> change behavior so as to comply with POSIX.
>> 
>> > I would not adjust the test case to cope with systems which are not in
>> > line with POSIX.
>> 
>> My understanding is that the libstdc++ testsuite is not meant to test
>> for POSIX conformance, but for conformance with the C++ language
>> standards.
>> 
>> C++ inherits rename from C, and C says the behavior is implementation
>> defined if the new name already exists.

> std::filesystem::rename is explicitly specified in terms of POSIX
> rename, not C rename.

Oh, sorry, I stand corrected.  I meant ::rename only.  My mind seems to
still resist the notion that <filesystem> is standard C++, rather than
an experimental extension proposal :-)

Anyway, I did not realize it deferred to POSIX semantics, even when
that's stricter than C standard.  Interesting...

> Instead, the implementation of std::filesystem::rename should have a
> special-case for rtems (and maybe other targets) that implements the
> POSIX rename semantics if calling ::rename isn't good enough.

Given what I've just learned, I agree.  Patch withdrawn.  I suppose the
other patch, on subdir renaming, must share the same fate.

/me longs for XFAILs for libstdc++ subtests ;-)
Alexandre Oliva June 23, 2022, 6:26 a.m. UTC | #5 
On Jun 22, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:

> "If the old argument and the new argument resolve to either the same
> existing directory entry or different directory entries for the same
> existing file, rename() shall return successfully and perform no other
> action." and "If the link named by the new argument exists, it shall
> be removed and old renamed to new."

> Instead, the implementation of std::filesystem::rename should have a
> special-case for rtems (and maybe other targets) that implements the
> POSIX rename semantics if calling ::rename isn't good enough.

Other POSIX requirements that make implementing them "interesting" when
::rename() doesn't are:

- leaving "new" alone when the operation fails: implies permissions,
  same filesystem, etc must all be checked first, atomically, before
  removing the file, and if renaming somehow fails after that, "new"
  must be somehow brought back to existence

- if "new" exists, it must keep on existing throughout the renaming
  process: implies temporary removal is not allowed, even if needed to
  to allow ::rename to succeed; they'd have to be part of the same
  filesystem transaction

- even if both of the above could somehow be satisfied, there are all
  sorts of race conditions that could enable pre-check to succeed, but
  that, even the checks were implemented fully and perfectly, would
  still enable the rename to fail after removal of a preexisting new.


Clearly something's gotta give.  Since ensuring the existence of "new"
throughout renaming is impossible if you can't rename to existing
filenames, I propose giving that up, though it's a very useful property.

I'd start by trying to rename old to new.  If that fails with EEXIST,
I'd rename new to a temporary name, and then try to rename old to new
and then remove the temporary name.

If renaming new to a temporary name fails, that means we can't remove it
either.

If renaming old to the now-vacated new still fails, that means we can't
rename old, so rename the temporary back to new.

If removing the temporary name for new fails, that means we couldn't
remove it to begin with, say because it's not empty, or because we lack
permission to remove it, so rename both back.


Knowing rename won't overwrite an existing pathname makes it easier to
pick a temporary name to use for the renaming: we know there's no risk
of overwriting files created concurrently, though we may have to try
other names if the rename-to-temp fails with EEXIST.

Creating a temporary directory sibling to new and picking a temporary
name in that directory enables us to detect early some cases of lack of
permission to remove new, that renaming it in the same directory
wouldn't hit.

Creating such a temporary directory also increases new/..'s link count
before moving it.  In a way, it checks for room for old/.., and it's no
worse than keeping new under the same parent, but picking a temporary
dir elsewhere would enable the renaming even if new/.. is already at the
limit.  Concurrent attempts to create directories in the same parent dir
could enable the operation to fail while also preventing the reversal of
the preparatory renames.  That would be a rarer occurrence if we rename
new to a temporary name under the same parent.


And all this doesn't even cover the case of moving old into itself,
which we'd also have to detect and prevent.


This feels more and more like a case for xfail until it gets fixed in
the kernel, where atomic filesystem operations belong :-(

Would a patch to add:

// { dg-xfail-if "::rename is not POSIX-compliant" { target *-*-rtems* } }

to rename.cc tests be acceptable?  I'm afraid I can't go further down
this rabbit hole, and my choices ATM seem to be limited to XFAIL
patches, whether accepted by the GCC community or carried internally.
Jonathan Wakely June 23, 2022, 9:25 a.m. UTC | #6 
On Thu, 23 Jun 2022 at 07:26, Alexandre Oliva wrote:
> This feels more and more like a case for xfail until it gets fixed in
> the kernel, where atomic filesystem operations belong :-(
>
> Would a patch to add:
>
> // { dg-xfail-if "::rename is not POSIX-compliant" { target *-*-rtems* } }
>
> to rename.cc tests be acceptable?  I'm afraid I can't go further down
> this rabbit hole, and my choices ATM seem to be limited to XFAIL
> patches, whether accepted by the GCC community or carried internally.


Yes, I think that's definitely the way to go.
Alexandre Oliva June 23, 2022, 11:21 a.m. UTC | #7 
On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:

> On Thu, 23 Jun 2022 at 07:26, Alexandre Oliva wrote:
>> Would a patch to add:
>> 
>> // { dg-xfail-if "::rename is not POSIX-compliant" { target *-*-rtems* } }
>> 
>> to rename.cc tests be acceptable?

> Yes, I think that's definitely the way to go.

The "target" above shouldn't have been there, and the :: appears to get
tcl/expect/dejagnu confused.  Here's the patch.

Regstrapped on x86_64-linux-gnu, also tested with a cross to
aarch64-rtems6.  Ok to install?


libstdc++: xfail rename tests on rtems

::rename on RTEMS does not meet several POSIX requirements, despite
compliance with C and C++ standards.  ::std::filesystem::rename, in
turn, has requirements borrowed from POSIX, so it would have to be a
lot more than a simple wrapper around ::rename on RTEMS, and even then
fall short.

Until RTEMS reimplements ::rename for POSIX compliance, expect
filesystem rename tests to fail on it.


for  libstdc++-v3/ChangeLog

	* testsuite/27_io/filesystem/operations/rename.cc: xfail on
	rtems.
	* testsuite/experimental/filesystem/operations/rename.cc:
	Likewise.
---
 .../27_io/filesystem/operations/rename.cc          |    1 +
 .../experimental/filesystem/operations/rename.cc   |    1 +
 2 files changed, 2 insertions(+)

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
index b74e1133a7618..62543158e5241 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
@@ -17,6 +17,7 @@
 
 // { dg-do run { target c++17 } }
 // { dg-require-filesystem-ts "" }
+// { dg-xfail-if "rename is not POSIX-compliant" { *-*-rtems* } }
 
 #include <filesystem>
 #include <testsuite_hooks.h>
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
index 37e743b770fdf..3c501757bff17 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
@@ -18,6 +18,7 @@
 // { dg-options "-DUSE_FILESYSTEM_TS -lstdc++fs" }
 // { dg-do run { target c++11 } }
 // { dg-require-filesystem-ts "" }
+// { dg-xfail-if "rename is not POSIX-compliant" { *-*-rtems* } }
 
 #include <experimental/filesystem>
 #include <testsuite_hooks.h>
Jonathan Wakely June 23, 2022, 11:38 a.m. UTC | #8 
On Thu, 23 Jun 2022 at 12:21, Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:
>
> > On Thu, 23 Jun 2022 at 07:26, Alexandre Oliva wrote:
> >> Would a patch to add:
> >>
> >> // { dg-xfail-if "::rename is not POSIX-compliant" { target *-*-rtems* } }
> >>
> >> to rename.cc tests be acceptable?
>
> > Yes, I think that's definitely the way to go.
>
> The "target" above shouldn't have been there, and the :: appears to get
> tcl/expect/dejagnu confused.  Here's the patch.
>
> Regstrapped on x86_64-linux-gnu, also tested with a cross to
> aarch64-rtems6.  Ok to install?

OK, thanks.


>
>
> libstdc++: xfail rename tests on rtems
>
> ::rename on RTEMS does not meet several POSIX requirements, despite
> compliance with C and C++ standards.  ::std::filesystem::rename, in
> turn, has requirements borrowed from POSIX, so it would have to be a
> lot more than a simple wrapper around ::rename on RTEMS, and even then
> fall short.
>
> Until RTEMS reimplements ::rename for POSIX compliance, expect
> filesystem rename tests to fail on it.
>
>
> for  libstdc++-v3/ChangeLog
>
>         * testsuite/27_io/filesystem/operations/rename.cc: xfail on
>         rtems.
>         * testsuite/experimental/filesystem/operations/rename.cc:
>         Likewise.
> ---
>  .../27_io/filesystem/operations/rename.cc          |    1 +
>  .../experimental/filesystem/operations/rename.cc   |    1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
> index b74e1133a7618..62543158e5241 100644
> --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
> +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
> @@ -17,6 +17,7 @@
>
>  // { dg-do run { target c++17 } }
>  // { dg-require-filesystem-ts "" }
> +// { dg-xfail-if "rename is not POSIX-compliant" { *-*-rtems* } }
>
>  #include <filesystem>
>  #include <testsuite_hooks.h>
> diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
> index 37e743b770fdf..3c501757bff17 100644
> --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
> +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
> @@ -18,6 +18,7 @@
>  // { dg-options "-DUSE_FILESYSTEM_TS -lstdc++fs" }
>  // { dg-do run { target c++11 } }
>  // { dg-require-filesystem-ts "" }
> +// { dg-xfail-if "rename is not POSIX-compliant" { *-*-rtems* } }
>
>  #include <experimental/filesystem>
>  #include <testsuite_hooks.h>
>
>
> --
> Alexandre Oliva, happy hacker                https://FSFLA.org/blogs/lxo/
>    Free Software Activist                       GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts.  Ask me about <https://stallmansupport.org>
>
Alexandre Oliva June 23, 2022, 12:41 p.m. UTC | #9 
On Jun 23, 2022, Jonathan Wakely <jwakely@redhat.com> wrote:

>> Regstrapped on x86_64-linux-gnu, also tested with a cross to
>> aarch64-rtems6.  Ok to install?

> OK, thanks.

Sorry, I failed to refresh this one too.
Here's what I'm going to install:

libstdc++: xfail rename tests on rtems

From: Alexandre Oliva <oliva@adacore.com>

::rename on RTEMS does not meet several POSIX requirements, despite
compliance with C and C++ standards.  ::std::filesystem::rename, in
turn, has requirements borrowed from POSIX, so it would have to be a
lot more than a simple wrapper around ::rename on RTEMS, and even then
fall short.

Until RTEMS reimplements ::rename for POSIX compliance, expect
filesystem rename tests to fail on it.


for  libstdc++-v3/ChangeLog

	* testsuite/27_io/filesystem/operations/rename.cc: xfail on
	rtems.
	* testsuite/experimental/filesystem/operations/rename.cc:
	Likewise.
---
 .../27_io/filesystem/operations/rename.cc          |    1 +
 .../experimental/filesystem/operations/rename.cc   |    1 +
 2 files changed, 2 insertions(+)

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
index b74e1133a7618..983374f42e448 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
@@ -17,6 +17,7 @@
 
 // { dg-do run { target c++17 } }
 // { dg-require-filesystem-ts "" }
+// { dg-xfail-run-if "rename is not POSIX-compliant" { *-*-rtems* } }
 
 #include <filesystem>
 #include <testsuite_hooks.h>
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
index 37e743b770fdf..762b943888f9e 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
@@ -18,6 +18,7 @@
 // { dg-options "-DUSE_FILESYSTEM_TS -lstdc++fs" }
 // { dg-do run { target c++11 } }
 // { dg-require-filesystem-ts "" }
+// { dg-xfail-run-if "rename is not POSIX-compliant" { *-*-rtems* } }
 
 #include <experimental/filesystem>
 #include <testsuite_hooks.h>
diff mbox series

Patch

diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
index 936e306041290..2fb2068dfd3c5 100644
--- a/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/rename.cc
@@ -46,14 +46,14 @@  test01()
 
   ec = bad_ec;
   std::ofstream{p1}; // create file
-  fs::rename(p1, p1, ec); // no-op
-  VERIFY( !ec );
+  fs::rename(p1, p1, ec); // no-op, but may fail
+  VERIFY( !ec || ec.value() == EEXIST );
   VERIFY( is_regular_file(p1) );
 
   ec.clear();
   rename(p2, p1, ec);
   VERIFY( ec );
-  VERIFY( ec.value() == ENOENT );
+  VERIFY( ec.value() == ENOENT || ec.value() == EEXIST );
   VERIFY( is_regular_file(p1) );
 
   ec = bad_ec;
@@ -65,10 +65,11 @@  test01()
   ec = bad_ec;
   std::ofstream{p1}; // create file
   fs::rename(p1, p2, ec);
-  VERIFY( !ec );
-  VERIFY( !exists(p1) );
+  VERIFY( !ec || ec.value() == EEXIST );
+  VERIFY( !exists(p1) || ec );
   VERIFY( is_regular_file(p2) );
 
+  fs::remove(p1, ec);
   fs::remove(p2, ec);
 }
 
diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
index 520d48ef8d844..d2175652a79a8 100644
--- a/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
+++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/rename.cc
@@ -47,8 +47,8 @@  test01()
 
   ec = bad_ec;
   std::ofstream{p1}; // create file
-  fs::rename(p1, p1, ec); // no-op
-  VERIFY( !ec );
+  fs::rename(p1, p1, ec); // no-op, but may fail
+  VERIFY( !ec || ec.value() == EEXIST );
   VERIFY( is_regular_file(p1) );
 
   ec.clear();
@@ -65,10 +65,11 @@  test01()
   ec = bad_ec;
   std::ofstream{p1}; // create file
   fs::rename(p1, p2, ec);
-  VERIFY( !ec );
-  VERIFY( !exists(p1) );
+  VERIFY( !ec || ec.value() == EEXIST );
+  VERIFY( !exists(p1) || ec );
   VERIFY( is_regular_file(p2) );
 
+  fs::remove(p1, ec);
   fs::remove(p2, ec);
 }