From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117755 invoked by alias); 24 Oct 2016 16:50:23 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 117729 invoked by uid 89); 24 Oct 2016 16:50:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=fs, iter, 169,7, 1697 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 24 Oct 2016 16:50:12 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 723D04DD49; Mon, 24 Oct 2016 16:50:11 +0000 (UTC) Received: from localhost (ovpn-116-29.ams2.redhat.com [10.36.116.29]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9OGoAPQ007982; Mon, 24 Oct 2016 12:50:10 -0400 Date: Mon, 24 Oct 2016 16:50:00 -0000 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [PATCH] Five patches for std::experimental::filesystem Message-ID: <20161024165010.GL2922@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="vDpvzslK0qRw06MN" Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.7.0 (2016-08-17) X-SW-Source: 2016-10/txt/msg01960.txt.bz2 --vDpvzslK0qRw06MN Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline Content-length: 2281 These implement DR resolutions and fix bugs. Fix error handling in filesystem::is_empty * src/filesystem/ops.cc (is_empty): Fix error handling. * testsuite/experimental/filesystem/operations/is_empty.cc: New test. PR71337 fix filesystem::temp_directory_path error handling PR libstdc++/71337 * src/filesystem/ops.cc (temp_directory_path): Pass error_code argument to other filesystem operations. * testsuite/experimental/filesystem/operations/temp_directory_path.cc: Add testcase for inaccessible directory. Make directory iterators become end iterator on error * src/filesystem/dir.cc (open_dir): Return same value for errors whether ignored or not. (_Dir::advance(error_code*, directory_options)): Return false on error. (directory_iterator(const path&, directory_options, error_code*)): Create end iterator on error (LWG 2723). (recursive_directory_iterator(const path&, directory_options, error_code*)): Likewise. * testsuite/experimental/filesystem/iterators/directory_iterator.cc: Update expected behaviour on error. * testsuite/experimental/filesystem/iterators/ recursive_directory_iterator.cc: Likewise. Do not retry failed close(3) in filesystem::copy * src/filesystem/ops.cc (close_fd): Remove. (do_copy_file): Just use close(3) instead of close_fd, to prevent retrying on error. Implement DR resolutions for filesystem::copy * src/filesystem/ops.cc (do_copy_file): Return an error if either source or destination is not a regular file. (copy): Update comment to refer to LWG 2681. Implement 2682 and 2683 resolutions. (read_symlink): Add missing ec.clear(). * testsuite/experimental/filesystem/operations/copy.cc: Update expected behaviour for copying directories with create_symlinks. Verify that error_code arguments are cleared if there's no error. * testsuite/experimental/filesystem/operations/read_symlink.cc: * New. Tested x86_64-linux, commiitted to trunk. I'm going to do some mass backports for all filesystem fixes to the branches this week as well. --vDpvzslK0qRw06MN Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="patch.txt" Content-length: 17856 commit 363cdc78be5edb191411e57206d94e18e42ec259 Author: Jonathan Wakely Date: Mon Oct 24 15:50:51 2016 +0100 Fix error handling in filesystem::is_empty * src/filesystem/ops.cc (is_empty): Fix error handling. * testsuite/experimental/filesystem/operations/is_empty.cc: New test. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 90c225b..d9a12df 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1022,20 +1022,24 @@ fs::hard_link_count(const path& p, error_code& ec) noexcept bool fs::is_empty(const path& p) { - return fs::is_directory(status(p)) - ? fs::directory_iterator(p) == fs::directory_iterator() - : fs::file_size(p) == 0; + error_code ec; + bool e = is_empty(p, ec); + if (ec) + _GLIBCXX_THROW_OR_ABORT(filesystem_error("cannot check is file is empty", + p, ec)); + return e; } bool fs::is_empty(const path& p, error_code& ec) noexcept { auto s = status(p, ec); - if (ec.value()) + if (ec) return false; - return fs::is_directory(s) + bool empty = fs::is_directory(s) ? fs::directory_iterator(p, ec) == fs::directory_iterator() : fs::file_size(p, ec) == 0; + return ec ? false : empty; } fs::file_time_type commit 4d32e1be1f623b42743092aefb7388019bea0c7f Author: Jonathan Wakely Date: Mon Oct 24 15:27:00 2016 +0100 PR71337 fix filesystem::temp_directory_path error handling PR libstdc++/71337 * src/filesystem/ops.cc (temp_directory_path): Pass error_code argument to other filesystem operations. * testsuite/experimental/filesystem/operations/temp_directory_path.cc: Add testcase for inaccessible directory. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index f8ba74e..90c225b 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -1428,12 +1428,17 @@ fs::path fs::temp_directory_path(error_code& ec) for (auto e = env; tmpdir == nullptr && *e != nullptr; ++e) tmpdir = ::getenv(*e); path p = tmpdir ? tmpdir : "/tmp"; - if (exists(p) && is_directory(p)) + auto st = status(p, ec); + if (!ec) { - ec.clear(); - return p; + if (is_directory(st)) + { + ec.clear(); + return p; + } + else + ec = std::make_error_code(std::errc::not_a_directory); } - ec = std::make_error_code(std::errc::not_a_directory); return {}; #endif } diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc index 6e202c9..7f7e9fd 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/temp_directory_path.cc @@ -45,6 +45,7 @@ test01() std::error_code ec; fs::path p1 = fs::temp_directory_path(ec); + VERIFY( !ec ); VERIFY( exists(p1) ); fs::path p2 = fs::temp_directory_path(); @@ -62,6 +63,7 @@ test02() std::error_code ec; fs::path p = fs::temp_directory_path(ec); VERIFY( ec ); + VERIFY( p == fs::path() ); std::error_code ec2; try { @@ -72,10 +74,54 @@ test02() VERIFY( ec2 == ec ); } +void +test03() +{ + auto p = __gnu_test::nonexistent_path(); + create_directories(p/"tmp"); + permissions(p, fs::perms::none); + setenv("TMPDIR", (p/"tmp").c_str(), 1); + std::error_code ec; + auto r = fs::temp_directory_path(ec); // libstdc++/PR71337 + VERIFY( ec == std::make_error_code(std::errc::permission_denied) ); + VERIFY( r == fs::path() ); + + std::error_code ec2; + try { + fs::temp_directory_path(); + } catch (const fs::filesystem_error& e) { + ec2 = e.code(); + } + VERIFY( ec2 == ec ); + + permissions(p, fs::perms::owner_all, ec); + remove_all(p, ec); +} + +void +test04() +{ + __gnu_test::scoped_file f; + setenv("TMPDIR", f.path.c_str(), 1); + std::error_code ec; + auto r = fs::temp_directory_path(ec); + VERIFY( ec == std::make_error_code(std::errc::not_a_directory) ); + VERIFY( r == fs::path() ); + + std::error_code ec2; + try { + fs::temp_directory_path(); + } catch (const fs::filesystem_error& e) { + ec2 = e.code(); + } + VERIFY( ec2 == ec ); +} int main() { test01(); test02(); + test03(); + test04(); } commit 015912fca75b8747123efbf5419e3e8be9b0a5b9 Author: Jonathan Wakely Date: Mon Oct 24 14:50:01 2016 +0100 Make directory iterators become end iterator on error * src/filesystem/dir.cc (open_dir): Return same value for errors whether ignored or not. (_Dir::advance(error_code*, directory_options)): Return false on error. (directory_iterator(const path&, directory_options, error_code*)): Create end iterator on error (LWG 2723). (recursive_directory_iterator(const path&, directory_options, error_code*)): Likewise. * testsuite/experimental/filesystem/iterators/directory_iterator.cc: Update expected behaviour on error. * testsuite/experimental/filesystem/iterators/ recursive_directory_iterator.cc: Likewise. diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc index 6ff12d0..4640d75 100644 --- a/libstdc++-v3/src/filesystem/dir.cc +++ b/libstdc++-v3/src/filesystem/dir.cc @@ -79,8 +79,7 @@ namespace return (obj & bits) != Bitmask::none; } - // Returns {dirp, p} on success, {nullptr, p} on error. - // If an ignored EACCES error occurs returns {}. + // Returns {dirp, p} on success, {} on error (whether ignored or not). inline fs::_Dir open_dir(const fs::path& p, fs::directory_options options, std::error_code* ec) @@ -102,7 +101,7 @@ namespace std::error_code(err, std::generic_category()))); ec->assign(err, std::generic_category()); - return {nullptr, p}; + return {}; } inline fs::file_type @@ -169,7 +168,7 @@ fs::_Dir::advance(error_code* ec, directory_options options) "directory iterator cannot advance", std::error_code(err, std::generic_category()))); ec->assign(err, std::generic_category()); - return true; + return false; } else { @@ -191,12 +190,6 @@ directory_iterator(const path& p, directory_options options, error_code* ec) if (sp->advance(ec, options)) _M_dir.swap(sp); } - else if (!dir.path.empty()) - { - // An error occurred, we need a non-empty shared_ptr so that *this will - // not compare equal to the end iterator. - _M_dir.reset(static_cast(nullptr)); - } } const fs::directory_entry& @@ -270,10 +263,6 @@ recursive_directory_iterator(const path& p, directory_options options, std::error_code(err, std::generic_category()))); ec->assign(err, std::generic_category()); - - // An error occurred, we need a non-empty shared_ptr so that *this will - // not compare equal to the end iterator. - _M_dirs.reset(static_cast<_Dir_stack*>(nullptr)); } } diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc index 5c80fb7..5788700 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc @@ -34,7 +34,7 @@ test01() const auto p = __gnu_test::nonexistent_path(); fs::directory_iterator iter(p, ec); VERIFY( ec ); - VERIFY( iter != fs::directory_iterator() ); + VERIFY( iter == fs::directory_iterator() ); // Test empty directory. create_directory(p, fs::current_path(), ec); @@ -58,7 +58,7 @@ test01() VERIFY( !ec ); iter = fs::directory_iterator(p, ec); VERIFY( ec ); - VERIFY( iter != fs::directory_iterator() ); + VERIFY( iter == fs::directory_iterator() ); // Test inaccessible directory, skipping permission denied. const auto opts = fs::directory_options::skip_permission_denied; diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc index 37b2606..b41c394 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc @@ -34,7 +34,7 @@ test01() const auto p = __gnu_test::nonexistent_path(); fs::recursive_directory_iterator iter(p, ec); VERIFY( ec ); - VERIFY( iter != fs::recursive_directory_iterator() ); + VERIFY( iter == fs::recursive_directory_iterator() ); // Test empty directory. create_directory(p, fs::current_path(), ec); @@ -60,7 +60,7 @@ test01() VERIFY( !ec ); iter = fs::recursive_directory_iterator(p, ec); VERIFY( ec ); - VERIFY( iter != fs::recursive_directory_iterator() ); + VERIFY( iter == fs::recursive_directory_iterator() ); // Test inaccessible directory, skipping permission denied. const auto opts = fs::directory_options::skip_permission_denied; commit ea465ff7461e814a8e2f83007653428a403eadb0 Author: Jonathan Wakely Date: Mon Oct 24 14:39:24 2016 +0100 Do not retry failed close(3) in filesystem::copy * src/filesystem/ops.cc (close_fd): Remove. (do_copy_file): Just use close(3) instead of close_fd, to prevent retrying on error. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 6f76053..f8ba74e 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -308,17 +308,6 @@ namespace return fs::file_time_type{seconds{s} + ns}; } - // Returns true if the file descriptor was successfully closed, - // otherwise returns false and the reason will be in errno. - inline bool - close_fd(int fd) - { - while (::close(fd)) - if (errno != EINTR) - return false; - return true; - } - bool do_copy_file(const fs::path& from, const fs::path& to, fs::copy_options option, @@ -405,8 +394,8 @@ namespace } struct CloseFD { - ~CloseFD() { if (fd != -1) close_fd(fd); } - bool close() { return close_fd(std::exchange(fd, -1)); } + ~CloseFD() { if (fd != -1) ::close(fd); } + bool close() { return ::close(std::exchange(fd, -1)) == 0; } int fd; }; commit 68eea18d5d6aa635f8d1b53e780e7fe703cd7610 Author: Jonathan Wakely Date: Mon Oct 24 14:33:27 2016 +0100 Implement DR resolutions for filesystem::copy * src/filesystem/ops.cc (do_copy_file): Return an error if either source or destination is not a regular file. (copy): Update comment to refer to LWG 2681. Implement 2682 and 2683 resolutions. (read_symlink): Add missing ec.clear(). * testsuite/experimental/filesystem/operations/copy.cc: Update expected behaviour for copying directories with create_symlinks. Verify that error_code arguments are cleared if there's no error. * testsuite/experimental/filesystem/operations/read_symlink.cc: New. diff --git a/libstdc++-v3/src/filesystem/ops.cc b/libstdc++-v3/src/filesystem/ops.cc index 2286e22..6f76053 100644 --- a/libstdc++-v3/src/filesystem/ops.cc +++ b/libstdc++-v3/src/filesystem/ops.cc @@ -361,6 +361,11 @@ namespace from_st = &st2; } f = make_file_status(*from_st); + if (!is_regular_file(f)) + { + ec = std::make_error_code(std::errc::not_supported); + return false; + } using opts = fs::copy_options; @@ -392,6 +397,11 @@ namespace ec = std::make_error_code(std::errc::file_exists); return false; } + else if (!is_regular_file(t)) + { + ec = std::make_error_code(std::errc::not_supported); + return false; + } } struct CloseFD { @@ -489,7 +499,8 @@ fs::copy(const path& from, const path& to, copy_options options, file_status f, t; stat_type from_st, to_st; - // N4099 doesn't check copy_symlinks here, but I think that's a defect. + // _GLIBCXX_RESOLVE_LIB_DEFECTS + // 2681. filesystem::copy() cannot copy symlinks if (use_lstat || copy_symlinks ? ::lstat(from.c_str(), &from_st) : ::stat(from.c_str(), &from_st)) @@ -556,6 +567,10 @@ fs::copy(const path& from, const path& to, copy_options options, do_copy_file(from, to, options, &from_st, ptr, ec); } } + // _GLIBCXX_RESOLVE_LIB_DEFECTS + // 2682. filesystem::copy() won't create a symlink to a directory + else if (is_directory(f) && create_symlinks) + ec = std::make_error_code(errc::is_a_directory); else if (is_directory(f) && (is_set(options, copy_options::recursive) || options == copy_options::none)) { @@ -568,7 +583,10 @@ fs::copy(const path& from, const path& to, copy_options options, for (const directory_entry& x : directory_iterator(from)) copy(x.path(), to/x.path().filename(), options, ec); } - // "Otherwise no effects." (should ec.clear() be called?) + // _GLIBCXX_RESOLVE_LIB_DEFECTS + // 2683. filesystem::copy() says "no effects" + else + ec.clear(); } bool @@ -1168,6 +1186,7 @@ fs::path fs::read_symlink(const path& p, error_code& ec) ec.assign(errno, std::generic_category()); return {}; } + ec.clear(); return path{buf.data(), buf.data()+len}; #else ec = std::make_error_code(std::errc::not_supported); diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc index 0d112a2..2cfb1c1 100644 --- a/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/copy.cc @@ -22,7 +22,6 @@ // 15.3 Copy [fs.op.copy] #include -#include #include #include @@ -43,14 +42,25 @@ test01() fs::copy(".", ".", fs::copy_options::none, ec); VERIFY( ec ); - std::ofstream{p.native()}; + __gnu_test::scoped_file f(p); VERIFY( fs::is_directory(".") ); VERIFY( fs::is_regular_file(p) ); ec.clear(); fs::copy(".", p, fs::copy_options::none, ec); VERIFY( ec ); - remove(p, ec); + auto to = __gnu_test::nonexistent_path(); + ec.clear(); + auto opts = fs::copy_options::create_symlinks; + fs::copy("/", to, opts, ec); + VERIFY( ec == std::make_error_code(std::errc::is_a_directory) ); + VERIFY( !exists(to) ); + + ec.clear(); + opts != fs::copy_options::recursive; + fs::copy("/", to, opts, ec); + VERIFY( ec == std::make_error_code(std::errc::is_a_directory) ); + VERIFY( !exists(to) ); } // Test is_symlink(f) case. @@ -59,29 +69,35 @@ test02() { auto from = __gnu_test::nonexistent_path(); auto to = __gnu_test::nonexistent_path(); - std::error_code ec; + std::error_code ec, bad = std::make_error_code(std::errc::invalid_argument); + ec = bad; fs::create_symlink(".", from, ec); VERIFY( !ec ); VERIFY( fs::exists(from) ); + ec = bad; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); VERIFY( !fs::exists(to) ); + ec = bad; fs::copy(from, to, fs::copy_options::skip_symlinks, ec); VERIFY( !ec ); VERIFY( !fs::exists(to) ); + ec = bad; fs::copy(from, to, fs::copy_options::skip_symlinks|fs::copy_options::copy_symlinks, ec); VERIFY( !ec ); VERIFY( !fs::exists(to) ); + ec = bad; fs::copy(from, to, fs::copy_options::copy_symlinks, ec); VERIFY( !ec ); VERIFY( fs::exists(to) ); + VERIFY( is_symlink(to) ); fs::copy(from, to, fs::copy_options::copy_symlinks, ec); VERIFY( ec ); @@ -129,10 +145,10 @@ void test05() { auto to = __gnu_test::nonexistent_path(); - std::error_code ec; + std::error_code ec = std::make_error_code(std::errc::invalid_argument); - fs::copy("/", to, fs::copy_options::create_symlinks, ec); - VERIFY( !ec ); + fs::copy("/", to, fs::copy_options::copy_symlinks, ec); + VERIFY( !ec ); // Previous value should be cleared (LWG 2683) } int diff --git a/libstdc++-v3/testsuite/experimental/filesystem/operations/read_symlink.cc b/libstdc++-v3/testsuite/experimental/filesystem/operations/read_symlink.cc new file mode 100644 index 0000000..c4137bd --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/filesystem/operations/read_symlink.cc @@ -0,0 +1,49 @@ +// Copyright (C) 2016 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 +// . + +// { dg-options "-lstdc++fs" } +// { dg-do run { target c++11 } } +// { dg-require-filesystem-ts "" } + +#include +#include +#include + +namespace fs = std::experimental::filesystem; + +void +test01() +{ + auto p = __gnu_test::nonexistent_path(); + std::error_code ec; + + read_symlink(p, ec); + VERIFY( ec ); + + fs::path tgt = "."; + create_symlink(tgt, p); + + auto result = read_symlink(p, ec); + VERIFY( !ec ); + VERIFY( result == tgt ); +} + +int +main() +{ + test01(); +} --vDpvzslK0qRw06MN--