diff mbox

Five patches for std::experimental::filesystem

Message ID 20161024165010.GL2922@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely Oct. 24, 2016, 4:50 p.m. UTC
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.
diff mbox

Patch

commit 363cdc78be5edb191411e57206d94e18e42ec259
Author: Jonathan Wakely <jwakely@redhat.com>
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 <jwakely@redhat.com>
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 <jwakely@redhat.com>
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<fs::_Dir*>(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 <jwakely@redhat.com>
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 <jwakely@redhat.com>
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 <experimental/filesystem>
-#include <fstream>
 #include <testsuite_fs.h>
 #include <testsuite_hooks.h>
 
@@ -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
+// <http://www.gnu.org/licenses/>.
+
+// { dg-options "-lstdc++fs" }
+// { dg-do run { target c++11 } }
+// { dg-require-filesystem-ts "" }
+
+#include <experimental/filesystem>
+#include <testsuite_hooks.h>
+#include <testsuite_fs.h>
+
+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();
+}