launchpad-reviewers team mailing list archive

[Gavin Panella <gavin.panella@xxxxxxxxxxxxx>]

Gavin Panella has proposed merging lp:~allenap/maas/oserror-filename-in-atomic-write--bug-1044512 into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)
Related bugs:
  Bug #1044512 in MAAS: "atomic_write should give path to attempted file on OSError"
  https://bugs.launchpad.net/maas/+bug/1044512

For more details, see:
https://code.launchpad.net/~allenap/maas/oserror-filename-in-atomic-write--bug-1044512/+merge/122372
-- 
https://code.launchpad.net/~allenap/maas/oserror-filename-in-atomic-write--bug-1044512/+merge/122372
Your team MAAS Maintainers is requested to review the proposed merge of lp:~allenap/maas/oserror-filename-in-atomic-write--bug-1044512 into lp:maas.

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-08-31 15:43:12 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-08-31 23:18:20 +0000
@@ -26,6 +26,7 @@
     Popen,
     )
 import sys
+import tempfile
 import time
 import types
 
@@ -135,6 +136,31 @@
         [recorded_mode] = recorded_modes
         self.assertEqual(mode, stat.S_IMODE(recorded_mode))
 
+    def test_atomic_write_sets_OSError_filename_if_undefined(self):
+        # When the filename attribute of an OSError is undefined when
+        # attempting to create a temporary file, atomic_write fills it in with
+        # a representative filename, similar to the specification required by
+        # mktemp(1).
+        mock_mkstemp = self.patch(tempfile, "mkstemp")
+        mock_mkstemp.side_effect = OSError()
+        filename = os.path.join("directory", "basename")
+        error = self.assertRaises(OSError, atomic_write, "content", filename)
+        self.assertEqual(
+            os.path.join("directory", ".basename.XXXXXX.tmp"),
+            error.filename)
+
+    def test_atomic_write_does_not_set_OSError_filename_if_defined(self):
+        # When the filename attribute of an OSError is defined when attempting
+        # to create a temporary file, atomic_write leaves it alone.
+        mock_mkstemp = self.patch(tempfile, "mkstemp")
+        mock_mkstemp.side_effect = OSError()
+        mock_mkstemp.side_effect.filename = factory.make_name("filename")
+        filename = os.path.join("directory", "basename")
+        error = self.assertRaises(OSError, atomic_write, "content", filename)
+        self.assertEqual(
+            mock_mkstemp.side_effect.filename,
+            error.filename)
+
 
 class TestIncrementalWrite(TestCase):
     """Test `incremental_write`."""

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-31 15:41:54 +0000
+++ src/provisioningserver/utils.py	2012-08-31 23:18:20 +0000
@@ -55,12 +55,20 @@
     # Write the file to a temporary place (next to the target destination,
     # to ensure that it is on the same filesystem).
     directory = os.path.dirname(filename)
-    temp_fd, temp_file = tempfile.mkstemp(
-        dir=directory, suffix=".tmp",
-        prefix=".%s." % os.path.basename(filename))
-    with os.fdopen(temp_fd, "wb") as f:
-        f.write(content)
-    return temp_file
+    prefix = ".%s." % os.path.basename(filename)
+    suffix = ".tmp"
+    try:
+        temp_fd, temp_file = tempfile.mkstemp(
+            dir=directory, suffix=suffix, prefix=prefix)
+    except OSError, error:
+        if error.filename is None:
+            error.filename = os.path.join(
+                directory, prefix + "XXXXXX" + suffix)
+        raise
+    else:
+        with os.fdopen(temp_fd, "wb") as f:
+            f.write(content)
+        return temp_file
 
 
 def atomic_write(content, filename, overwrite=True, mode=0600):