duplicity-team team mailing list archive

[Michael Terry <michael.terry@xxxxxxxxxxxxx>]

Michael Terry has proposed merging lp:~mterry/duplicity/catch-seq-copy-error into lp:duplicity.

Requested reviews:
  duplicity-team (duplicity-team)

For more details, see:
https://code.launchpad.net/~mterry/duplicity/catch-seq-copy-error/+merge/186106

When converting a sequence of patches to a ROPath, sometimes duplicity wants to collapse the current set of patches into a temporary file for librsync's benefit, because it has encountered a non-true-file-object and librsync only works on true-file-objects.

When this happens, if an error occurs, the exception is not caught and will terminate duplicity.

If that same error happened lazily -- as it normally does if duplicity doesn't decide to collapse patches into a temporary file -- the exception is caught, like many others, by IterTreeReducer's handlers and is merely logged while the rest of duplicity chugs along.

Ideally these two cases would be handled more similarly.  It makes bugs like bug 662442 much worse than they need to be because a problem with a single file prevents restoring the whole backup.

This branch captures common errors that occur during the patch-collapsing, logs them, and moves on.  Behavior is still not *identical* between the two cases [1], but it's much better.

[1] For example, if you are restoring only one file, the fact that this branch treats files with collapsing problems as non-existent means that duplicity will say it didn't find any files to restore -- whereas the normal path will recognize that it at least found a file.  This didn't seem like a huge problem and would have been a little awkward to fix (maybe have seq2ropath return a ticking-time-bomb ROPath that would raise its error next time it was used?  Seemed hackier than this branch).
-- 
https://code.launchpad.net/~mterry/duplicity/catch-seq-copy-error/+merge/186106
Your team duplicity-team is requested to review the proposed merge of lp:~mterry/duplicity/catch-seq-copy-error into lp:duplicity.

=== modified file 'duplicity/patchdir.py'
--- duplicity/patchdir.py	2012-09-11 14:11:39 +0000
+++ duplicity/patchdir.py	2013-09-17 17:12:24 +0000
@@ -29,6 +29,7 @@
 from duplicity import log #@UnusedImport
 from duplicity import diffdir
 from duplicity import misc
+from duplicity import robust
 from duplicity import selection
 from duplicity import tempdir
 from duplicity import util #@UnusedImport
@@ -457,6 +458,12 @@
         i -= 1
     return result_list
 
+def patch_iter_error_handler(exc, *args):
+    # Just return, caller will handle appropriately (we'd handle it ourselves,
+    # but we don't have access to the path index, which we'd want for proper
+    # logging.
+    return exc
+
 def patch_seq2ropath( patch_seq ):
     """Apply the patches in patch_seq, return single ropath"""
     first = patch_seq[0]
@@ -466,6 +473,7 @@
         assert len( patch_seq ) == 1, len( patch_seq )
         return first.get_ropath()
 
+    result = patch_seq[-1].get_ropath()
     current_file = first.open( "rb" )
 
     for delta_ropath in patch_seq[1:]:
@@ -476,13 +484,28 @@
             by using the duplicity.tempdir to tell us where.
             """
             tempfp = tempfile.TemporaryFile( dir=tempdir.default().dir() )
-            misc.copyfileobj( current_file, tempfp )
+            error = robust.check_common_error(patch_iter_error_handler,
+                                              misc.copyfileobj,
+                                              [current_file, tempfp])
+            if isinstance(error, Exception):
+                # Could not copy from current file, which generally means that
+                # we could not create the file from previous patches.  A
+                # librsync error perhaps.  We'll warn about it, but no reason
+                # to stop the entire restore.  Just don't write the file.
+                log.Warn(_("Error '%s' patching %s, ") % 
+                         (str(error), delta_ropath.get_relative_path()),
+                         log.WarningCode.cannot_process,
+                         util.escape(delta_ropath.get_relative_path()))
+                tempfp.close()
+                current_file.close()
+                result.blank() # pretend file doesn't exist
+                return result
             assert not current_file.close()
             tempfp.seek( 0 )
             current_file = tempfp
         current_file = librsync.PatchedFile( current_file,
                                             delta_ropath.open( "rb" ) )
-    result = patch_seq[-1].get_ropath()
+
     result.setfileobj( current_file )
     return result
 

=== modified file 'testing/tests/patchdirtest.py'
--- testing/tests/patchdirtest.py	2011-11-04 14:47:03 +0000
+++ testing/tests/patchdirtest.py	2013-09-17 17:12:24 +0000
@@ -299,6 +299,30 @@
         testseq([self.snapshot(), self.delta1(), self.delta2()], ("%s 644" % ids),
                 "3499 34957839485792357 458348573")
 
+    def test_patch_seq2ropath_copy_failure(self):
+        # First, create a non-file basis file.  This will trigger the code
+        # path that copies the non-file into a real file.  Then, expect an
+        # empty path result, because the copy threw an error.  Notably, we
+        # don't want the error to bubble up -- seq2ropath should shield us from
+        # that).
+        class FakeFile:
+            raised = False
+            closed = False
+            def read(self, num_bytes):
+                self.raised = True
+                raise librsync.librsyncError("DUPTEST")
+            def close(self):
+                self.closed = True
+
+        ss = self.out.append("snapshot")
+        ss.setfileobj(FakeFile())
+        ss.difftype = "snapshot"
+        ss.type = "reg"
+
+        result = patchdir.patch_seq2ropath([ss, self.delta1()])
+        assert not result.exists()
+        assert ss.fileobj.raised
+        assert ss.fileobj.closed
 
 if __name__ == "__main__":
     unittest.main()