| Message ID | 20180104230027.GA24800@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR libstdc++/83626 Don't throw for remove("") and remove_all("") | expand |
What if the file to be removed is externally removed between the symlink_status and the ::remove call? This is probably QoI because filesystem race, but it seems reasonable to double check errno if ::remove fails and not fail if the failure is due to the file not existing.
On 04/01/18 21:02 -0500, Tim Song wrote: >What if the file to be removed is externally removed between the >symlink_status and the ::remove call? This is probably QoI because >filesystem race, but it seems reasonable to double check errno if >::remove fails and not fail if the failure is due to the file not >existing. Yes, the race makes it undefined, but checking for ENOENT seems reasonable. Thanks for the suggestion.
On 05/01/18 10:37 +0000, Jonathan Wakely wrote: >On 04/01/18 21:02 -0500, Tim Song wrote: >>What if the file to be removed is externally removed between the >>symlink_status and the ::remove call? This is probably QoI because >>filesystem race, but it seems reasonable to double check errno if >>::remove fails and not fail if the failure is due to the file not >>existing. > >Yes, the race makes it undefined, but checking for ENOENT seems >reasonable. Thanks for the suggestion. This makes remove and remove_all handle (some) filesystem races, by ignoring ENOENT errors when removing entries or while iterating over sub-directories. It also avoids redundant stat() calls in remove_all, and fixes a bug in the return value of the throwing version of remove_all. Tested powerpc64le-linux, committed to trunk. I've attached a multithreaded test I used to test the handling of filesystem races, but I'm not committing that test. commit 64a3b77dd050d16069061185c3efef020698fca4 Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Jan 5 16:39:46 2018 +0000 PR libstdc++/83626 handle ENOENT due to filesystem race PR libstdc++/83626 * src/filesystem/ops.cc (remove(const path&, error_code&)): Do not report an error for ENOENT. (remove_all(const path&)): Fix type of result variable. (remove_all(const path&, error_code&)): Use non-throwing increment for directory iterator. Call POSIX remove directly to avoid redundant calls to symlink_status. Do not report errors for ENOENT. * src/filesystem/std-ops.cc: Likewise. * testsuite/27_io/filesystem/operations/remove_all.cc: Test throwing overload. * testsuite/experimental/filesystem/operations/remove_all.cc: Likewise. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 5a7cb599bb6..3690fb94d63 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1025,13 +1025,16 @@ fs::remove(const path& p, error_code& ec) noexcept ec.clear(); return false; // Nothing to do, not an error. } - if (::remove(p.c_str()) != 0) + if (::remove(p.c_str()) == 0) { - ec.assign(errno, std::generic_category()); - return false; + ec.clear(); + return true; } - ec.clear(); - return true; + else if (errno == ENOENT) + ec.clear(); + else + ec.assign(errno, std::generic_category()); + return false; } @@ -1039,7 +1042,7 @@ std::uintmax_t fs::remove_all(const path& p) { error_code ec; - bool result = remove_all(p, ec); + const auto result = remove_all(p, ec); if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); return result; @@ -1051,15 +1054,29 @@ fs::remove_all(const path& p, error_code& ec) noexcept const auto s = symlink_status(p, ec); if (!status_known(s)) return -1; + ec.clear(); + if (s.type() == file_type::not_found) + return 0; + uintmax_t count = 0; if (s.type() == file_type::directory) - for (directory_iterator d(p, ec), end; !ec && d != end; ++d) - count += fs::remove_all(d->path(), ec); - if (!ec && fs::remove(p, ec)) + { + for (directory_iterator d(p, ec), end; !ec && d != end; d.increment(ec)) + count += fs::remove_all(d->path(), ec); + if (ec.value() == ENOENT) + ec.clear(); + else if (ec) + return -1; + } + + if (::remove(p.c_str()) == 0) ++count; - if (ec) - return -1; + else if (errno != ENOENT) + { + ec.assign(errno, std::generic_category()); + return -1; + } return count; } diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc index 6ff280a9c76..2411bbb7977 100644 --- a/libstdc++-v3/src/filesystem/std-ops.cc +++ b/libstdc++-v3/src/filesystem/std-ops.cc @@ -1274,13 +1274,16 @@ fs::remove(const path& p, error_code& ec) noexcept ec.clear(); return false; // Nothing to do, not an error. } - if (::remove(p.c_str()) != 0) + if (::remove(p.c_str()) == 0) { - ec.assign(errno, std::generic_category()); - return false; + ec.clear(); + return true; } - ec.clear(); - return true; + else if (errno == ENOENT) + ec.clear(); + else + ec.assign(errno, std::generic_category()); + return false; } @@ -1288,7 +1291,7 @@ std::uintmax_t fs::remove_all(const path& p) { error_code ec; - bool result = remove_all(p, ec); + const auto result = remove_all(p, ec); if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); return result; @@ -1300,15 +1303,29 @@ fs::remove_all(const path& p, error_code& ec) const auto s = symlink_status(p, ec); if (!status_known(s)) return -1; + ec.clear(); + if (s.type() == file_type::not_found) + return 0; + uintmax_t count = 0; if (s.type() == file_type::directory) - for (directory_iterator d(p, ec), end; !ec && d != end; ++d) - count += fs::remove_all(d->path(), ec); - if (!ec && fs::remove(p, ec)) + { + for (directory_iterator d(p, ec), end; !ec && d != end; d.increment(ec)) + count += fs::remove_all(d->path(), ec); + if (ec.value() == ENOENT) + ec.clear(); + else if (ec) + return -1; + } + + if (::remove(p.c_str()) == 0) ++count; - if (ec) - return -1; + else if (errno != ENOENT) + { + ec.assign(errno, std::generic_category()); + return -1; + } return count; } diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc index 8ee5c4d39c1..633cde57243 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc @@ -85,8 +85,28 @@ test01() b2.path.clear(); } +void +test02() +{ + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b/c"); + std::uintmax_t n = remove_all(dir/"a"); + VERIFY( n == 3 ); + VERIFY( exists(dir) ); + VERIFY( !exists(dir/"a") ); + + n = remove_all(dir/"a"); + VERIFY( n == 0 ); + VERIFY( exists(dir) ); + + n = remove_all(dir); + VERIFY( n == 1 ); + VERIFY( !exists(dir) ); +} + int main() { test01(); + test02(); } diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc index fa146b4317c..67f6e989d27 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc @@ -85,8 +85,28 @@ test01() b2.path.clear(); } +void +test02() +{ + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b/c"); + std::uintmax_t n = remove_all(dir/"a"); + VERIFY( n == 3 ); + VERIFY( exists(dir) ); + VERIFY( !exists(dir/"a") ); + + n = remove_all(dir/"a"); + VERIFY( n == 0 ); + VERIFY( exists(dir) ); + + n = remove_all(dir); + VERIFY( n == 1 ); + VERIFY( !exists(dir) ); +} + int main() { test01(); + test02(); }
On 05/01/18 18:02 +0000, Jonathan Wakely wrote: >On 05/01/18 10:37 +0000, Jonathan Wakely wrote: >>On 04/01/18 21:02 -0500, Tim Song wrote: >>>What if the file to be removed is externally removed between the >>>symlink_status and the ::remove call? This is probably QoI because >>>filesystem race, but it seems reasonable to double check errno if >>>::remove fails and not fail if the failure is due to the file not >>>existing. >> >>Yes, the race makes it undefined, but checking for ENOENT seems >>reasonable. Thanks for the suggestion. > >This makes remove and remove_all handle (some) filesystem races, by >ignoring ENOENT errors when removing entries or while iterating over >sub-directories. > >It also avoids redundant stat() calls in remove_all, and fixes a bug >in the return value of the throwing version of remove_all. > >Tested powerpc64le-linux, committed to trunk. > >I've attached a multithreaded test I used to test the handling of >filesystem races, but I'm not committing that test. In Bugzilla the reporter pointed out that the call to symlink_status in filesystem::remove is now redundant, because handling the ENOENT case after calling ::remove gives the same result (and is less vulnerable to TOCTTOU races). This removes the symlink_status call, and then makes remove_all call filesystem::remove again instead of ::remove, because doing so doesn't add an unnecessary symlink_status call now. Tested powerpc64le-linux, committed to trunk. commit 89cfa63335230a0ce82152171cdbd2bb5be64440 Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Jan 5 23:28:06 2018 +0000 PR libstdc++/83626 simplify filesystem::remove and filesystem::remove_all PR libstdc++/83626 * src/filesystem/ops.cc (remove(const path&, error_code&)): Remove unnecessary symlink_status call. (remove_all(const path&, error_code&)): Use filesystem::remove. * src/filesystem/std-ops.cc: Likewise. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 3690fb94d63..899defea6d2 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1017,14 +1017,6 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - const auto s = symlink_status(p, ec); - if (!status_known(s)) - return false; - if (s.type() == file_type::not_found) - { - ec.clear(); - return false; // Nothing to do, not an error. - } if (::remove(p.c_str()) == 0) { ec.clear(); @@ -1070,14 +1062,9 @@ fs::remove_all(const path& p, error_code& ec) noexcept return -1; } - if (::remove(p.c_str()) == 0) + if (fs::remove(p, ec)) ++count; - else if (errno != ENOENT) - { - ec.assign(errno, std::generic_category()); - return -1; - } - return count; + return ec ? -1 : count; } void diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc index bed1ad1fe27..65b06f5b67b 100644 --- a/libstdc++-v3/src/filesystem/std-ops.cc +++ b/libstdc++-v3/src/filesystem/std-ops.cc @@ -1254,7 +1254,7 @@ bool fs::remove(const path& p) { error_code ec; - bool result = fs::remove(p, ec); + const bool result = fs::remove(p, ec); if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove", p, ec)); return result; @@ -1263,14 +1263,6 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - const auto s = symlink_status(p, ec); - if (!status_known(s)) - return false; - if (s.type() == file_type::not_found) - { - ec.clear(); - return false; // Nothing to do, not an error. - } if (::remove(p.c_str()) == 0) { ec.clear(); @@ -1316,14 +1308,9 @@ fs::remove_all(const path& p, error_code& ec) return -1; } - if (::remove(p.c_str()) == 0) + if (fs::remove(p, ec)) ++count; - else if (errno != ENOENT) - { - ec.assign(errno, std::generic_category()); - return -1; - } - return count; + return ec ? -1 : count; } void
diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 56b1432036f..5a7cb599bb6 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1009,7 +1009,7 @@ fs::remove(const path& p) { error_code ec; bool result = fs::remove(p, ec); - if (ec.value()) + if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove", p, ec)); return result; } @@ -1017,17 +1017,21 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - if (exists(symlink_status(p, ec))) + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return false; + if (s.type() == file_type::not_found) { - if (::remove(p.c_str()) == 0) - { - ec.clear(); - return true; - } - else - ec.assign(errno, std::generic_category()); + ec.clear(); + return false; // Nothing to do, not an error. } - return false; + if (::remove(p.c_str()) != 0) + { + ec.assign(errno, std::generic_category()); + return false; + } + ec.clear(); + return true; } @@ -1036,7 +1040,7 @@ fs::remove_all(const path& p) { error_code ec; bool result = remove_all(p, ec); - if (ec.value()) + if (ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot remove all", p, ec)); return result; } @@ -1044,14 +1048,19 @@ fs::remove_all(const path& p) std::uintmax_t fs::remove_all(const path& p, error_code& ec) noexcept { - auto fs = symlink_status(p, ec); - uintmax_t count = 0; - if (ec.value() == 0 && fs.type() == file_type::directory) - for (directory_iterator d(p, ec), end; ec.value() == 0 && d != end; ++d) - count += fs::remove_all(d->path(), ec); - if (ec.value()) + const auto s = symlink_status(p, ec); + if (!status_known(s)) return -1; - return fs::remove(p, ec) ? ++count : -1; // fs:remove() calls ec.clear() + ec.clear(); + uintmax_t count = 0; + if (s.type() == file_type::directory) + for (directory_iterator d(p, ec), end; !ec && d != end; ++d) + count += fs::remove_all(d->path(), ec); + if (!ec && fs::remove(p, ec)) + ++count; + if (ec) + return -1; + return count; } void diff --git a/libstdc++-v3/src/filesystem/std-ops.cc b/libstdc++-v3/src/filesystem/std-ops.cc index 8392f6fc835..6ff280a9c76 100644 --- a/libstdc++-v3/src/filesystem/std-ops.cc +++ b/libstdc++-v3/src/filesystem/std-ops.cc @@ -1266,17 +1266,21 @@ fs::remove(const path& p) bool fs::remove(const path& p, error_code& ec) noexcept { - if (exists(symlink_status(p, ec))) + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return false; + if (s.type() == file_type::not_found) { - if (::remove(p.c_str()) == 0) - { - ec.clear(); - return true; - } - else - ec.assign(errno, std::generic_category()); + ec.clear(); + return false; // Nothing to do, not an error. } - return false; + if (::remove(p.c_str()) != 0) + { + ec.assign(errno, std::generic_category()); + return false; + } + ec.clear(); + return true; } @@ -1293,14 +1297,19 @@ fs::remove_all(const path& p) std::uintmax_t fs::remove_all(const path& p, error_code& ec) { - auto fs = symlink_status(p, ec); + const auto s = symlink_status(p, ec); + if (!status_known(s)) + return -1; + ec.clear(); uintmax_t count = 0; - if (!ec && fs.type() == file_type::directory) + if (s.type() == file_type::directory) for (directory_iterator d(p, ec), end; !ec && d != end; ++d) count += fs::remove_all(d->path(), ec); + if (!ec && fs::remove(p, ec)) + ++count; if (ec) return -1; - return fs::remove(p, ec) ? ++count : -1; // fs:remove() calls ec.clear() + return count; } void diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc new file mode 100644 index 00000000000..ef9f06d7300 --- /dev/null +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove.cc @@ -0,0 +1,100 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-options "-std=gnu++17 -lstdc++fs" } +// { dg-do run { target c++17 } } +// { dg-require-filesystem-ts "" } + +#include <filesystem> +#include <testsuite_hooks.h> +#include <testsuite_fs.h> + +namespace fs = std::filesystem; + +void +test01() +{ + std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); + bool n; + + n = fs::remove("", ec); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( !n ); + + auto p = __gnu_test::nonexistent_path(); + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( !n ); + + auto link = __gnu_test::nonexistent_path(); + create_symlink(p, link); // dangling symlink + ec = bad_ec; + n = remove(link, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); + + __gnu_test::scoped_file f(p); + create_symlink(p, link); + ec = bad_ec; + n = remove(link, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but + VERIFY( exists(p) ); // its target is not. + + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(p)) ); + + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b"); + ec.clear(); + n = remove(dir/"a", ec); + VERIFY( ec ); + VERIFY( !n ); + VERIFY( exists(dir/"a/b") ); + + permissions(dir, fs::perms::none, ec); + if (!ec) + { + ec.clear(); + n = remove(dir/"a/b", ec); + VERIFY( ec ); + VERIFY( !n ); + permissions(dir, fs::perms::owner_all, ec); + } + + ec = bad_ec; + n = remove(dir/"a/b", ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(dir/"a/b") ); + + remove(dir/"a", ec); + remove(dir, ec); +} + +int +main() +{ + test01(); +} diff --git a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc index 9cdc86fe9c3..8ee5c4d39c1 100644 --- a/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/27_io/filesystem/operations/remove_all.cc @@ -29,19 +29,19 @@ void test01() { std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); std::uintmax_t n; n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( n == 0 ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); + ec = bad_ec; n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); + VERIFY( n == 0 ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; @@ -59,7 +59,7 @@ test01() VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); + const auto dir = __gnu_test::nonexistent_path(); create_directories(dir/"a/b/c"); ec = bad_ec; n = remove_all(dir/"a", ec); diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc new file mode 100644 index 00000000000..8d1801099a2 --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove.cc @@ -0,0 +1,100 @@ +// Copyright (C) 2018 Free Software Foundation, Inc. +// +// This file is part of the GNU ISO C++ Library. This library is free +// software; you can redistribute it and/or modify it under the +// terms of the GNU General Public License as published by the +// Free Software Foundation; either version 3, or (at your option) +// any later version. + +// This library is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License along +// with this library; see the file COPYING3. If not see +// <http://www.gnu.org/licenses/>. + +// { dg-options "-lstdc++fs" } +// { dg-do run { target c++11 } } +// { dg-require-filesystem-ts "" } + +#include <filesystem> +#include <testsuite_hooks.h> +#include <testsuite_fs.h> + +namespace fs = std::experimental::filesystem; + +void +test01() +{ + std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); + bool n; + + n = fs::remove("", ec); + VERIFY( !ec ); // This seems odd, but is what the standard requires. + VERIFY( !n ); + + auto p = __gnu_test::nonexistent_path(); + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( !n ); + + auto link = __gnu_test::nonexistent_path(); + create_symlink(p, link); // dangling symlink + ec = bad_ec; + n = remove(link, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); + + __gnu_test::scoped_file f(p); + create_symlink(p, link); + ec = bad_ec; + n = remove(link, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but + VERIFY( exists(p) ); // its target is not. + + ec = bad_ec; + n = remove(p, ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(symlink_status(p)) ); + + const auto dir = __gnu_test::nonexistent_path(); + create_directories(dir/"a/b"); + ec.clear(); + n = remove(dir/"a", ec); + VERIFY( ec ); + VERIFY( !n ); + VERIFY( exists(dir/"a/b") ); + + permissions(dir, fs::perms::none, ec); + if (!ec) + { + ec.clear(); + n = remove(dir/"a/b", ec); + VERIFY( ec ); + VERIFY( !n ); + permissions(dir, fs::perms::owner_all, ec); + } + + ec = bad_ec; + n = remove(dir/"a/b", ec); + VERIFY( !ec ); + VERIFY( n ); + VERIFY( !exists(dir/"a/b") ); + + remove(dir/"a", ec); + remove(dir, ec); +} + +int +main() +{ + test01(); +} diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc index 983998d78fa..fa146b4317c 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/remove_all.cc @@ -29,19 +29,19 @@ void test01() { std::error_code ec; + const std::error_code bad_ec = make_error_code(std::errc::invalid_argument); std::uintmax_t n; n = fs::remove_all("", ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); // This seems odd, but is what the TS requires. + VERIFY( n == 0 ); auto p = __gnu_test::nonexistent_path(); - ec.clear(); + ec = bad_ec; n = remove_all(p, ec); - VERIFY( ec ); - VERIFY( n == std::uintmax_t(-1) ); + VERIFY( !ec ); + VERIFY( n == 0 ); - const auto bad_ec = ec; auto link = __gnu_test::nonexistent_path(); create_symlink(p, link); // dangling symlink ec = bad_ec; @@ -59,7 +59,7 @@ test01() VERIFY( !exists(symlink_status(link)) ); // The symlink is removed, but VERIFY( exists(p) ); // its target is not. - auto dir = __gnu_test::nonexistent_path(); + const auto dir = __gnu_test::nonexistent_path(); create_directories(dir/"a/b/c"); ec = bad_ec; n = remove_all(dir/"a", ec);
This fixes filesystem::remove and filesystem::remove_all for the cases where the file to be removed doesn't exist. The standard says it should not be an error, but we were reporting errors. PR libstdc++/83626 * src/filesystem/ops.cc (remove(const path&, error_code&))): Remove redundant call to ec.clear(). (remove_all(const path&, error_code&))): Do not return an error for non-existent paths. * src/filesystem/std-ops.cc: Likewise. * testsuite/27_io/filesystem/operations/remove.cc: New test. * testsuite/27_io/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths. * testsuite/experimental/filesystem/operations/remove.cc: New test. * testsuite/experimental/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths. Tested powerpc64le-linux, committed to trunk. Backports to follow. commit 7733d104f219cc453de45d5196720e24174a98d0 Author: Jonathan Wakely <jwakely@redhat.com> Date: Thu Jan 4 13:28:22 2018 +0000 PR libstdc++/83626 Don't throw for remove("") and remove_all("") PR libstdc++/83626 * src/filesystem/ops.cc (remove(const path&, error_code&))): Remove redundant call to ec.clear(). (remove_all(const path&, error_code&))): Do not return an error for non-existent paths. * src/filesystem/std-ops.cc: Likewise. * testsuite/27_io/filesystem/operations/remove.cc: New test. * testsuite/27_io/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths. * testsuite/experimental/filesystem/operations/remove.cc: New test. * testsuite/experimental/filesystem/operations/remove_all.cc: Fix expected results for non-existent paths.