[Boost-commit] svn:boost r53073 - in trunk: boost/filesystem libs/filesystem/doc libs/filesystem/src libs/filesystem/test

Subject: [Boost-commit] svn:boost r53073 - in trunk: boost/filesystem libs/filesystem/doc libs/filesystem/src libs/filesystem/test
From: bdawes_at_[hidden]
Date: 2009-05-17 11:55:47

Author: bemandawes
Date: 2009-05-17 11:55:46 EDT (Sun, 17 May 2009)
New Revision: 53073
URL: http://svn.boost.org/trac/boost/changeset/53073

Log:
Fix Filesystem #2925, copy_file atomiticity
Text files modified: 
   trunk/boost/filesystem/operations.hpp          |    14 ++++++++++----                          
   trunk/libs/filesystem/doc/reference.html       |    17 +++++++++++++----                       
   trunk/libs/filesystem/src/operations.cpp       |    36 ++++++++++++++++++++++--------------    
   trunk/libs/filesystem/test/operations_test.cpp |    16 ++++++++++++++++                        
   4 files changed, 61 insertions(+), 22 deletions(-)

Modified: trunk/boost/filesystem/operations.hpp
==============================================================================
--- trunk/boost/filesystem/operations.hpp	(original)
+++ trunk/boost/filesystem/operations.hpp	2009-05-17 11:55:46 EDT (Sun, 17 May 2009)
@@ -15,6 +15,7 @@
 #define BOOST_FILESYSTEM_OPERATIONS_HPP
 
 #include <boost/filesystem/path.hpp>
+#include <boost/detail/scoped_enum_emulation.hpp>
 
 #include <boost/shared_ptr.hpp>
 #include <boost/utility/enable_if.hpp>
@@ -182,7 +183,7 @@
       BOOST_FILESYSTEM_DECL system::error_code
         rename_api( const std::string & from, const std::string & to );
       BOOST_FILESYSTEM_DECL system::error_code
-        copy_file_api( const std::string & from, const std::string & to );
+        copy_file_api( const std::string & from, const std::string & to, bool fail_if_exists );
 
 #   if defined(BOOST_WINDOWS_API)
       
@@ -226,7 +227,7 @@
       BOOST_FILESYSTEM_DECL system::error_code
         rename_api( const std::wstring & from, const std::wstring & to );
       BOOST_FILESYSTEM_DECL system::error_code
-        copy_file_api( const std::wstring & from, const std::wstring & to );
+        copy_file_api( const std::wstring & from, const std::wstring & to, bool fail_if_exists );
 
 #     endif
 #   endif
@@ -506,11 +507,16 @@
           from_path, to_path, ec ) );
     }
 
-    BOOST_FS_FUNC(void) copy_file( const Path & from_path, const Path & to_path )
+    BOOST_SCOPED_ENUM_START(copy_option)
+      { fail_if_exists, overwrite_if_exists };
+    BOOST_SCOPED_ENUM_END
+
+    BOOST_FS_FUNC(void) copy_file( const Path & from_path, const Path & to_path,
+      BOOST_SCOPED_ENUM(copy_option) option = copy_option::fail_if_exists )
     {
       system::error_code ec( detail::copy_file_api(
         from_path.external_directory_string(),
-        to_path.external_directory_string() ) );
+        to_path.external_directory_string(), option == copy_option::fail_if_exists ) );
       if ( ec )
         boost::throw_exception( basic_filesystem_error<Path>(
           "boost::filesystem::copy_file",

Modified: trunk/libs/filesystem/doc/reference.html
==============================================================================
--- trunk/libs/filesystem/doc/reference.html	(original)
+++ trunk/libs/filesystem/doc/reference.html	2009-05-17 11:55:46 EDT (Sun, 17 May 2009)
@@ -410,8 +410,15 @@
 </span>      template &lt;class Path&gt; bool remove(const Path&amp; p);
       template &lt;class Path1, class Path2&gt;
         void rename(const Path1&amp; from_p, const Path2&amp; to_p);
+
+      BOOST_SCOPED_ENUM_START(<a name="copy_option">copy_option</a>)
+        { fail_if_exists, overwrite_if_exists };
+      BOOST_SCOPED_ENUM_END
+
       template &lt;class Path1, class Path2&gt;
-        void copy_file(const Path1&amp; from_fp, const Path2&amp; to_fp);
+        void copy_file(const Path1&amp; from_fp, const Path2&amp; to_fp,
+          BOOST_SCOPED_ENUM(copy_option) option=copy_option::fail_if_exists);
+
       template &lt;class Path&gt; Path system_complete(const Path&amp; p);
       template &lt;class Path&gt; Path complete(const Path&amp; p, const Path&amp; base=initial_path&lt;Path&gt;());
 
@@ -2249,7 +2256,9 @@
   existing file, it is removed. A symbolic link is itself renamed, rather than 
   the file it resolves to being renamed. <i>-- end note</i>]</p>
 </blockquote>
-<pre>template &lt;class Path1, class Path2&gt; void copy_file(const Path1&amp; from_fp, const Path2&amp; to_fp);</pre>
+<pre>template &lt;class Path1, class Path2&gt;
+  void copy_file(const Path1&amp; from_fp, const Path2&amp; to_fp,
+                 BOOST_SCOPED_ENUM(copy_option) option=copy_option::fail_if_exists);</pre>
 <blockquote>
   <p><i>Requires:</i> <code>Path1::external_string_type</code> and <code>
   Path2::external_string_type</code> are the same type. </p>
@@ -2257,7 +2266,7 @@
   resolves to are copied to the file <code>to_fp</code> resolves to.</p>
   <p><i>Throws:</i> <code>basic_filesystem_error&lt;Path&gt;</code> if <code>
   from_fp.empty() || to_fp.empty() ||!exists(from_fp) || !is_regular_file(from_fp) 
-  || exists(to_fp)</code></p>
+  || (option==copy_option::fail_if_exists &amp;&amp; exists(to_fp))</code></p>
 </blockquote>
 <pre>template &lt;class Path&gt; Path complete(const Path&amp; p, const Path&amp; base=initial_path&lt;Path&gt;());</pre>
 <blockquote>
@@ -3076,7 +3085,7 @@
 <p>Distributed under the Boost Software License, Version 1.0. See
 <a href="http://www.boost.org/LICENSE_1_0.txt">www.boost.org/LICENSE_1_0.txt</a></p>
 <p>Revised
-<!--webbot bot="Timestamp" S-Type="EDITED" S-Format="%d %B %Y" startspan -->13 October 2008<!--webbot bot="Timestamp" endspan i-checksum="32600" --></p>
+<!--webbot bot="Timestamp" S-Type="EDITED" S-Format="%d %B %Y" startspan -->17 May 2009<!--webbot bot="Timestamp" endspan i-checksum="15147" --></p>
 
 </body>
 

Modified: trunk/libs/filesystem/src/operations.cpp
==============================================================================
--- trunk/libs/filesystem/src/operations.cpp	(original)
+++ trunk/libs/filesystem/src/operations.cpp	2009-05-17 11:55:46 EDT (Sun, 17 May 2009)
@@ -706,9 +706,9 @@
       }
 
       BOOST_FILESYSTEM_DECL error_code
-      copy_file_api( const std::wstring & from, const std::wstring & to )
+      copy_file_api( const std::wstring & from, const std::wstring & to, bool fail_if_exists )
       {
-        return error_code( ::CopyFileW( from.c_str(), to.c_str(), /*fail_if_exists=*/true )
+        return error_code( ::CopyFileW( from.c_str(), to.c_str(), fail_if_exists )
           ? 0 : ::GetLastError(), system_category );
       }
 
@@ -886,9 +886,9 @@
       }
 
       BOOST_FILESYSTEM_DECL error_code
-      copy_file_api( const std::string & from, const std::string & to )
+      copy_file_api( const std::string & from, const std::string & to, bool fail_if_exists )
       {
-        return error_code( ::CopyFileA( from.c_str(), to.c_str(), /*fail_if_exists=*/true )
+        return error_code( ::CopyFileA( from.c_str(), to.c_str(), fail_if_exists )
           ? 0 : ::GetLastError(), system_category );
       }
 
@@ -1203,22 +1203,30 @@
 
       BOOST_FILESYSTEM_DECL error_code
       copy_file_api( const std::string & from_file_ph,
-        const std::string & to_file_ph )
+        const std::string & to_file_ph, bool fail_if_exists )
       {
         const std::size_t buf_sz = 32768;
         boost::scoped_array<char> buf( new char [buf_sz] );
         int infile=-1, outfile=-1;  // -1 means not open
+
+        // bug fixed: code previously did a stat() on the from_file first, but that
+        // introduced a gratuitous race condition; the stat() is now done after the open()
+
+        if ( (infile = ::open( from_file_ph.c_str(), O_RDONLY )) < 0 )
+          { return error_code( errno, system_category ); }
+
         struct stat from_stat;
+        if ( ::stat( from_file_ph.c_str(), &from_stat ) != 0 )
+          { return error_code( errno, system_category ); }
 
-        if ( ::stat( from_file_ph.c_str(), &from_stat ) != 0
-          || (infile = ::open( from_file_ph.c_str(),
-                              O_RDONLY )) < 0
-          || (outfile = ::open( to_file_ph.c_str(),
-                                O_WRONLY | O_CREAT | O_EXCL,
-                                from_stat.st_mode )) < 0 )
-        {
-          if ( infile >= 0 ) ::close( infile );
-          return error_code( errno, system_category );
+        int oflag = O_CREAT | O_WRONLY;
+        if ( fail_if_exists ) oflag |= O_EXCL;
+        if (  (outfile = ::open( to_file_ph.c_str(), oflag, from_stat.st_mode )) < 0 )
+        {
+          int open_errno = errno;
+          BOOST_ASSERT( infile >= 0 );
+          ::close( infile );
+          return error_code( open_errno, system_category );
         }
 
         ssize_t sz, sz_read=1, sz_write;

Modified: trunk/libs/filesystem/test/operations_test.cpp
==============================================================================
--- trunk/libs/filesystem/test/operations_test.cpp	(original)
+++ trunk/libs/filesystem/test/operations_test.cpp	2009-05-17 11:55:46 EDT (Sun, 17 May 2009)
@@ -711,6 +711,22 @@
   BOOST_TEST( fs::exists( d1 / "f2" ) );
   BOOST_TEST( !fs::is_directory( d1 / "f2" ) );
   verify_file( d1 / "f2", "foobar1" );
+
+  bool copy_ex_ok = false;
+  try { fs::copy_file( file_ph, d1 / "f2" ); }
+  catch ( const fs::filesystem_error & ) { copy_ex_ok = true; }
+  BOOST_TEST( copy_ex_ok );
+
+  copy_ex_ok = false;
+  try { fs::copy_file( file_ph, d1 / "f2", fs::copy_option::fail_if_exists ); }
+  catch ( const fs::filesystem_error & ) { copy_ex_ok = true; }
+  BOOST_TEST( copy_ex_ok );
+
+  copy_ex_ok = true;
+  try { fs::copy_file( file_ph, d1 / "f2", fs::copy_option::overwrite_if_exists ); }
+  catch ( const fs::filesystem_error & ) { copy_ex_ok = false; }
+  BOOST_TEST( copy_ex_ok );
+
   std::cout << "copy_file test complete" << std::endl;
 
   // rename() test case numbers refer to operations.htm#rename table