From patchwork Wed Sep 23 11:39:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 521647 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id B5E9914012C for ; Wed, 23 Sep 2015 21:39:56 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=IGMo0hPh; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=qUfqwFxsZKoIUc70LbfCBcg03uMO3lPZpDHK/6Wu6KFPMMCNpxYfW P81iuFunF5orJLZXh4Fdu61EzD4TrGnS7+rrysenQ9nEmZFIc7tDBb/tagSsoyVq j1NCY2B6csl5PJnjEsDiFJ5zVJMVGkGR+sWlwbdwRc9k1eK3KeMR+8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=Uy3NLHRL6K1zbNEZ18apACxPn0g=; b=IGMo0hPhHgjbnJ0XL7ky ifuWJ5kRT5ADrAPAqSBgMUu6OjEFITcy38/jvGCEd3zJzV3gjDbaO5H28qiby7H+ qZfBeyhxWVJOLdPgITbY+hQ2hvVQKBGYyTxkrKuUEG5YWlaSRD2IVWTio1Wqjsqz H2ra4JVPEq08eRz4DDU9ESo= Received: (qmail 27878 invoked by alias); 23 Sep 2015 11:39:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 27860 invoked by uid 89); 23 Sep 2015 11:39:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 23 Sep 2015 11:39:27 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id D7B42C0B9190; Wed, 23 Sep 2015 11:39:25 +0000 (UTC) Received: from localhost (ovpn-116-78.ams2.redhat.com [10.36.116.78]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8NBdO3F025890; Wed, 23 Sep 2015 07:39:25 -0400 Date: Wed, 23 Sep 2015 12:39:24 +0100 From: Jonathan Wakely To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [v3 patch] Fix Filesystem TS directory iterators Message-ID: <20150923113924.GW2969@redhat.com> MIME-Version: 1.0 Content-Disposition: inline X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.5.23 (2014-03-12) directory_iterator and recursive_directory_iterator fail to meet this requirement in http://wg21.link/n4099#Class-directory_iterator The directory_iterator default constructor shall create an iterator equal to the end iterator value, and this shall be the only valid iterator for the end condition. The current code creates the end iterator when an error occurs during construction and an error_code parameter was used (so an exception is not thrown, but construction finishes normally and sets the error_code). This fixes it by creating a distinct error state that is not the end iterator state: // 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)); This way the shared_ptr owns a null pointer, so (bool)_M_dir is false (and we don't allow incrementing or dereferencing) but it can be distinguished from an empty shared_ptr by comparing them using shared_ptr::owner_before. (The order of the owner_before checks is chosen so that the common case of testing iter != directory_iterator() should short-circuit and only check the first condition). There were a few other problems with directory iterators, including the fact that the get_file_type function never worked because autoconf was defining _GLIBCXX_GLIBCXX_HAVE_STRUCT_DIRENT_D_TYPE instead of the macro I was checking, _GLIBCXX_HAVE_STRUCT_DIRENT_D_TYPE. I've removed the ErrorCode utility that was meant to simplify clearing/setting an error_code that may or may not be present, but really just obsfuscated things. I'm also now consistently checking the skip_permission_denied flag everywhere it matters. Tested x86_64-linux, powerpc64le-linux, x86_64-dragonfly4.1, committed to trunk. commit 8d08e1c6724cb433e1ca4f975ce85bd277ba2389 Author: Jonathan Wakely Date: Wed Sep 23 00:28:19 2015 +0100 Fix semantics of Filesystem TS directory iterators [class.directory_iterator] p4 and [directory_iterator.members] p4 require that only the default constructor and ignored permission denied errors can create the end iterator. * acinclude.m4 (GLIBCXX_CHECK_FILESYSTEM_DEPS): Remove _GLIBCXX_ prefix from HAVE_STRUCT_DIRENT_D_TYPE. * config.h.in: Regenerate. * configure: Regenerate. * include/experimental/fs_dir.h (operator==, operator==): Use owner_before instead of pointer equality. (directory_iterator(std::shared_ptr<_Dir>, error_code*)): Remove. * src/filesystem/dir.cc (ErrorCode): Remove. (_Dir::advance): Change ErrorCode parameter to error_code*, add directory_options parameter and check it on error. (opendir): Rename to open_dir to avoid clashing with macro. Change ErrorCode parameter to error_code*. (make_shared_dir): Remove. (native_readdir) [_GLIBCXX_FILESYSTEM_IS_WINDOWS]: Don't set errno. (directory_iterator(std::shared_ptr<_Dir>, error_code*)): Remove. (directory_iterator(const path&, directory_options, error_code*)): Pass options to _Dir::advance and create non-end iterator on error. (recursive_directory_iterator(const path&, directory_options, error_code*)): Clear error_code on ignored error, create non-end iterator otherwise. (recursive_directory_iterator::increment): Pass _M_options to _Dir::advance. (recursive_directory_iterator::pop): Likewise. * testsuite/experimental/filesystem/iterators/directory_iterator.cc: New. * testsuite/experimental/filesystem/iterators/ recursive_directory_iterator.cc: New. diff --git a/libstdc++-v3/acinclude.m4 b/libstdc++-v3/acinclude.m4 index c133c25..4b031f7 100644 --- a/libstdc++-v3/acinclude.m4 +++ b/libstdc++-v3/acinclude.m4 @@ -3940,7 +3940,7 @@ dnl [glibcxx_cv_dirent_d_type=no]) ]) if test $glibcxx_cv_dirent_d_type = yes; then - AC_DEFINE(_GLIBCXX_HAVE_STRUCT_DIRENT_D_TYPE, 1, [Define to 1 if `d_type' is a member of `struct dirent'.]) + AC_DEFINE(HAVE_STRUCT_DIRENT_D_TYPE, 1, [Define to 1 if `d_type' is a member of `struct dirent'.]) fi AC_MSG_RESULT($glibcxx_cv_dirent_d_type) dnl diff --git a/libstdc++-v3/include/experimental/fs_dir.h b/libstdc++-v3/include/experimental/fs_dir.h index d46d41b..0c5253f 100644 --- a/libstdc++-v3/include/experimental/fs_dir.h +++ b/libstdc++-v3/include/experimental/fs_dir.h @@ -201,14 +201,12 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 return __tmp; } - friend bool - operator==(const directory_iterator& __lhs, - const directory_iterator& __rhs) - { return __lhs._M_dir == __rhs._M_dir; } - private: directory_iterator(const path&, directory_options, error_code*); - directory_iterator(std::shared_ptr<_Dir>, error_code*); + + friend bool + operator==(const directory_iterator& __lhs, + const directory_iterator& __rhs); friend class recursive_directory_iterator; @@ -222,6 +220,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 end(directory_iterator) { return directory_iterator(); } inline bool + operator==(const directory_iterator& __lhs, const directory_iterator& __rhs) + { + return !__rhs._M_dir.owner_before(__lhs._M_dir) + && !__lhs._M_dir.owner_before(__rhs._M_dir); + } + + inline bool operator!=(const directory_iterator& __lhs, const directory_iterator& __rhs) { return !(__lhs == __rhs); } @@ -287,14 +292,13 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 void disable_recursion_pending() { _M_pending = false; } - friend bool - operator==(const recursive_directory_iterator& __lhs, - const recursive_directory_iterator& __rhs) - { return __lhs._M_dirs == __rhs._M_dirs; } - private: recursive_directory_iterator(const path&, directory_options, error_code*); + friend bool + operator==(const recursive_directory_iterator& __lhs, + const recursive_directory_iterator& __rhs); + struct _Dir_stack; std::shared_ptr<_Dir_stack> _M_dirs; directory_options _M_options; @@ -308,6 +312,14 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 end(recursive_directory_iterator) { return recursive_directory_iterator(); } inline bool + operator==(const recursive_directory_iterator& __lhs, + const recursive_directory_iterator& __rhs) + { + return !__rhs._M_dirs.owner_before(__lhs._M_dirs) + && !__lhs._M_dirs.owner_before(__rhs._M_dirs); + } + + inline bool operator!=(const recursive_directory_iterator& __lhs, const recursive_directory_iterator& __rhs) { return !(__lhs == __rhs); } diff --git a/libstdc++-v3/src/filesystem/dir.cc b/libstdc++-v3/src/filesystem/dir.cc index 016a78d..bce751c 100644 --- a/libstdc++-v3/src/filesystem/dir.cc +++ b/libstdc++-v3/src/filesystem/dir.cc @@ -43,28 +43,6 @@ namespace fs = std::experimental::filesystem; -namespace -{ - struct ErrorCode - { - ErrorCode(std::error_code* p) : ec(p) { } - - ErrorCode(ErrorCode&& e) : ec(std::exchange(e.ec, nullptr)) { } - - ~ErrorCode() { if (ec) ec->clear(); } - - void assign(int err) - { - ec->assign(err, std::generic_category()); - ec = nullptr; - } - - explicit operator bool() { return ec != nullptr; } - - std::error_code* ec; - }; -} - struct fs::_Dir { _Dir() : dirp(nullptr) { } @@ -80,7 +58,7 @@ struct fs::_Dir ~_Dir() { if (dirp) ::closedir(dirp); } - bool advance(ErrorCode); + bool advance(std::error_code*, directory_options = directory_options::none); DIR* dirp; fs::path path; @@ -96,9 +74,14 @@ namespace return (obj & bits) != Bitmask::none; } + // Returns {dirp, p} on success, {nullptr, p} on error. + // If an ignored EACCES error occurs returns {}. fs::_Dir - opendir(const fs::path& p, fs::directory_options options, ErrorCode ec) + open_dir(const fs::path& p, fs::directory_options options, std::error_code* ec) { + if (ec) + ec->clear(); + if (DIR* dirp = ::opendir(p.c_str())) return {dirp, p}; @@ -112,16 +95,8 @@ namespace "directory iterator cannot open directory", p, std::error_code(err, std::generic_category()))); - ec.assign(err); - return {}; - } - - inline std::shared_ptr - make_shared_dir(fs::_Dir&& dir) - { - if (dir.dirp) - return std::make_shared(std::move(dir)); - return {}; + ec->assign(err, std::generic_category()); + return {nullptr, p}; } inline fs::file_type @@ -158,7 +133,6 @@ namespace native_readdir(DIR* dirp, ::dirent*& entryp) { #ifdef _GLIBCXX_FILESYSTEM_IS_WINDOWS - errno = 0; if ((entryp = ::readdir(dirp))) return 0; return errno; @@ -168,25 +142,34 @@ namespace } } +// Returns false when the end of the directory entries is reached. +// Reports errors by setting ec or throwing. bool -fs::_Dir::advance(ErrorCode ec) +fs::_Dir::advance(error_code* ec, directory_options options) { + if (ec) + ec->clear(); + ::dirent ent; ::dirent* result = &ent; if (int err = native_readdir(dirp, result)) { + if (err == EACCES + && is_set(options, directory_options::skip_permission_denied)) + return false; + if (!ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error( "directory iterator cannot advance", std::error_code(err, std::generic_category()))); - ec.assign(err); + ec->assign(err, std::generic_category()); return true; } else if (result != nullptr) { // skip past dot and dot-dot if (!strcmp(ent.d_name, ".") || !strcmp(ent.d_name, "..")) - return advance(std::move(ec)); + return advance(ec, options); entry = fs::directory_entry{path / ent.d_name}; type = get_file_type(ent); return true; @@ -202,15 +185,21 @@ fs::_Dir::advance(ErrorCode ec) fs::directory_iterator:: directory_iterator(const path& p, directory_options options, error_code* ec) -: directory_iterator(make_shared_dir(opendir(p, options, ec)), ec) -{ } - -fs::directory_iterator:: -directory_iterator(std::shared_ptr<_Dir> dir, error_code* ec) -: _M_dir(std::move(dir)) { - if (_M_dir && !_M_dir->advance(ec)) - _M_dir.reset(); + _Dir dir = open_dir(p, options, ec); + + if (dir.dirp) + { + auto sp = std::make_shared(std::move(dir)); + 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& @@ -257,7 +246,7 @@ struct fs::recursive_directory_iterator::_Dir_stack : std::stack<_Dir> fs::recursive_directory_iterator:: recursive_directory_iterator(const path& p, directory_options options, - error_code* ec) + error_code* ec) : _M_options(options), _M_pending(true) { if (DIR* dirp = ::opendir(p.c_str())) @@ -272,7 +261,11 @@ recursive_directory_iterator(const path& p, directory_options options, const int err = errno; if (err == EACCES && is_set(options, fs::directory_options::skip_permission_denied)) - return; + { + if (ec) + ec->clear(); + return; + } if (!ec) _GLIBCXX_THROW_OR_ABORT(filesystem_error( @@ -280,6 +273,10 @@ 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)); } } @@ -358,21 +355,14 @@ fs::recursive_directory_iterator::increment(error_code& ec) noexcept if (std::exchange(_M_pending, true) && recurse(top, _M_options, ec)) { - _Dir dir = opendir(top.entry.path(), _M_options, &ec); - if (ec.value()) + _Dir dir = open_dir(top.entry.path(), _M_options, &ec); + if (ec) return *this; if (dir.dirp) - { _M_dirs->push(std::move(dir)); - if (!_M_dirs->top().advance(&ec)) // dir is empty - pop(); - return *this; - } - // else skip permission denied and continue in parent dir } - ec.clear(); - while (!_M_dirs->top().advance(&ec) && !ec.value()) + while (!_M_dirs->top().advance(&ec, _M_options) && !ec) { _M_dirs->pop(); if (_M_dirs->empty()) @@ -399,5 +389,5 @@ fs::recursive_directory_iterator::pop() _M_dirs.reset(); return; } - } while (!_M_dirs->top().advance(nullptr)); + } while (!_M_dirs->top().advance(nullptr, _M_options)); } diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc new file mode 100644 index 0000000..56b808d --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/directory_iterator.cc @@ -0,0 +1,77 @@ +// Copyright (C) 2015 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 "-std=gnu++11 -lstdc++fs" } +// { dg-require-filesystem-ts "" } + +#include +#include +#include + +namespace fs = std::experimental::filesystem; + +void +test01() +{ + bool test __attribute__((unused)) = false; + std::error_code ec; + + // Test non-existent path. + const auto p = __gnu_test::nonexistent_path(); + fs::directory_iterator iter(p, ec); + VERIFY( ec ); + VERIFY( iter != fs::directory_iterator() ); + + // Test empty directory. + create_directory(p, fs::current_path(), ec); + VERIFY( !ec ); + iter = fs::directory_iterator(p, ec); + VERIFY( !ec ); + VERIFY( iter == fs::directory_iterator() ); + + // Test non-empty directory. + create_directory_symlink(p, p / "l", ec); + VERIFY( !ec ); + iter = fs::directory_iterator(p, ec); + VERIFY( !ec ); + VERIFY( iter != fs::directory_iterator() ); + VERIFY( iter->path() == p/"l" ); + ++iter; + VERIFY( iter == fs::directory_iterator() ); + + // Test inaccessible directory. + permissions(p, fs::perms::none, ec); + VERIFY( !ec ); + iter = fs::directory_iterator(p, ec); + VERIFY( ec ); + VERIFY( iter != fs::directory_iterator() ); + + // Test inaccessible directory, skipping permission denied. + const auto opts = fs::directory_options::skip_permission_denied; + iter = fs::directory_iterator(p, opts, ec); + VERIFY( !ec ); + VERIFY( iter == fs::directory_iterator() ); + + permissions(p, fs::perms::owner_all, ec); + remove_all(p, ec); +} + +int +main() +{ + test01(); +} diff --git a/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc new file mode 100644 index 0000000..9424c80 --- /dev/null +++ b/libstdc++-v3/testsuite/experimental/filesystem/iterators/recursive_directory_iterator.cc @@ -0,0 +1,104 @@ +// Copyright (C) 2015 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 "-std=gnu++11 -lstdc++fs" } +// { dg-require-filesystem-ts "" } + +#include +#include +#include + +namespace fs = std::experimental::filesystem; + +void +test01() +{ + bool test __attribute__((unused)) = false; + std::error_code ec; + + // Test non-existent path. + const auto p = __gnu_test::nonexistent_path(); + fs::recursive_directory_iterator iter(p, ec); + VERIFY( ec ); + VERIFY( iter != fs::recursive_directory_iterator() ); + + // Test empty directory. + create_directory(p, fs::current_path(), ec); + VERIFY( !ec ); + iter = fs::recursive_directory_iterator(p, ec); + VERIFY( !ec ); + VERIFY( iter == fs::recursive_directory_iterator() ); + + // Test non-empty directory. + create_directories(p / "d1/d2"); + VERIFY( !ec ); + iter = fs::recursive_directory_iterator(p, ec); + VERIFY( !ec ); + VERIFY( iter != fs::recursive_directory_iterator() ); + VERIFY( iter->path() == p/"d1" ); + ++iter; + VERIFY( iter->path() == p/"d1/d2" ); + ++iter; + VERIFY( iter == fs::recursive_directory_iterator() ); + + // Test inaccessible directory. + permissions(p, fs::perms::none, ec); + VERIFY( !ec ); + iter = fs::recursive_directory_iterator(p, ec); + VERIFY( ec ); + VERIFY( iter != fs::recursive_directory_iterator() ); + + // Test inaccessible directory, skipping permission denied. + const auto opts = fs::directory_options::skip_permission_denied; + iter = fs::recursive_directory_iterator(p, opts, ec); + VERIFY( !ec ); + VERIFY( iter == fs::recursive_directory_iterator() ); + + // Test inaccessible sub-directory. + permissions(p, fs::perms::owner_all, ec); + VERIFY( !ec ); + permissions(p/"d1/d2", fs::perms::none, ec); + VERIFY( !ec ); + iter = fs::recursive_directory_iterator(p, ec); + VERIFY( !ec ); + VERIFY( iter != fs::recursive_directory_iterator() ); + VERIFY( iter->path() == p/"d1" ); + ++iter; // should recurse into d1 + VERIFY( iter->path() == p/"d1/d2" ); + iter.increment(ec); // should fail to recurse into p/d1/d2 + VERIFY( ec ); + + // Test inaccessible sub-directory, skipping permission denied. + iter = fs::recursive_directory_iterator(p, opts, ec); + VERIFY( !ec ); + VERIFY( iter != fs::recursive_directory_iterator() ); + VERIFY( iter->path() == p/"d1" ); + ++iter; // should recurse into d1 + VERIFY( iter->path() == p/"d1/d2" ); + iter.increment(ec); // should fail to recurse into p/d1/d2, so skip it + VERIFY( !ec ); + VERIFY( iter == fs::recursive_directory_iterator() ); + + permissions(p/"d1/d2", fs::perms::owner_all, ec); + remove_all(p, ec); +} + +int +main() +{ + test01(); +}