[Branch,~linaro-maintainers/linaro-image-tools/trunk] Rev 281: Make sure we cleanup properly when install_hwpacks fails

Message ID 20110201173129.11543.61609.launchpad@loganberry.canonical.com
State Accepted
Headers show

Commit Message

Guilherme Salgado Feb. 1, 2011, 5:31 p.m.
Merge authors:
  Andy Doan (doanac)
  Guilherme Salgado (salgado)
Related merge proposals:
  https://code.launchpad.net/~doanac/linaro-image-tools/slash-proc-spam-version2/+merge/47436
  proposed by: Andy Doan (doanac)
  review: Approve - Guilherme Salgado (salgado)
------------------------------------------------------------
revno: 281 [merge]
committer: Guilherme Salgado <salgado@canonical.com>
branch nick: trunk
timestamp: Tue 2011-02-01 15:29:25 -0200
message:
  Make sure we cleanup properly when install_hwpacks fails
modified:
  linaro_media_create/hwpack.py
  linaro_media_create/tests/test_media_create.py


--
lp:linaro-image-tools
https://code.launchpad.net/~linaro-maintainers/linaro-image-tools/trunk

You are subscribed to branch lp:linaro-image-tools.
To unsubscribe from this branch go to https://code.launchpad.net/~linaro-maintainers/linaro-image-tools/trunk/+edit-subscription

Patch

=== modified file 'linaro_media_create/hwpack.py'
--- linaro_media_create/hwpack.py	2011-01-28 19:50:48 +0000
+++ linaro_media_create/hwpack.py	2011-02-01 17:07:04 +0000
@@ -18,6 +18,7 @@ 
 # along with Linaro Image Tools.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import sys
 
 from linaro_media_create import cmd_runner
 from linaro_media_create.utils import is_arm_host
@@ -49,12 +50,12 @@ 
     copy_file(linaro_hwpack_install_path,
               os.path.join(chroot_dir, 'usr', 'bin'))
 
-    mount_chroot_proc(chroot_dir)
-
-    for hwpack_file in hwpack_files:
-        install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
-
-    run_local_atexit_funcs()
+    try:
+        mount_chroot_proc(chroot_dir)
+        for hwpack_file in hwpack_files:
+            install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes)
+    finally:
+        run_local_atexit_funcs()
 
 
 def install_hwpack(chroot_dir, hwpack_file, hwpack_force_yes):
@@ -82,12 +83,14 @@ 
     Also register a function in local_atexit to unmount that /proc filesystem.
     """
     chroot_proc = os.path.join(chroot_dir, 'proc')
+
+    def umount_chroot_proc():
+        cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
+    local_atexit.append(umount_chroot_proc)
+
     proc = cmd_runner.run(
         ['mount', 'proc', chroot_proc, '-t', 'proc'], as_root=True)
     proc.wait()
-    def umount_chroot_proc():
-        cmd_runner.run(['umount', '-v', chroot_proc], as_root=True).wait()
-    local_atexit.append(umount_chroot_proc)
 
 
 def copy_file(filepath, directory):
@@ -130,5 +133,18 @@ 
 
 def run_local_atexit_funcs():
     # Run the funcs in LIFO order, just like atexit does.
+    exc_info = None
     while len(local_atexit) > 0:
-        local_atexit.pop()()
+        func = local_atexit.pop()
+        try:
+            func()
+        except SystemExit:
+            exc_info = sys.exc_info()
+        except:
+            import traceback
+            print >> sys.stderr, "Error in local_atexit:"
+            traceback.print_exc()
+            exc_info = sys.exc_info()
+
+    if exc_info is not None:
+        raise exc_info[0], exc_info[1], exc_info[2]

=== modified file 'linaro_media_create/tests/test_media_create.py'
--- linaro_media_create/tests/test_media_create.py	2011-02-01 17:23:43 +0000
+++ linaro_media_create/tests/test_media_create.py	2011-02-01 17:29:25 +0000
@@ -1109,3 +1109,64 @@ 
              ['sudo', 'mv', '-f', '/tmp/dir/hosts', 'chroot/etc'],
              ['sudo', 'mv', '-f', '/tmp/dir/resolv.conf', 'chroot/etc']],
             fixture.mock.calls)
+
+    def test_run_local_atexit_funcs(self):
+        self.useFixture(MockSomethingFixture(
+            sys, 'stderr', open('/dev/null', 'w')))
+        self.call_order = []
+        class TestException(Exception):
+            pass
+        def raising_func():
+            self.call_order.append('raising_func')
+            raise TestException()
+        def behaving_func():
+            self.call_order.append('behaving_func')
+            self.behaving_func_called = True
+        # run_local_atexit_funcs() runs the atexit handlers in LIFO order, but
+        # even though the first function called (raising_func) will raise
+        # an exception, the second one will still be called after it.
+        linaro_media_create.hwpack.local_atexit = [
+            behaving_func, raising_func]
+        # run_local_atexit_funcs() also propagates the last exception raised
+        # by one of the functions.
+        self.assertRaises(
+            TestException, linaro_media_create.hwpack.run_local_atexit_funcs)
+        self.assertEquals(
+            ['raising_func', 'behaving_func'], self.call_order)
+
+    def test_hwpack_atexit(self):
+        self.run_local_atexit_functions_called = False
+
+        def mock_run_local_atexit_functions():
+            self.run_local_atexit_functions_called = True
+
+        def mock_install_hwpack(p1, p2, p3):
+            raise Exception('hwpack mock exception')
+
+        self.useFixture(MockSomethingFixture(
+            sys, 'stdout', open('/dev/null', 'w')))
+        self.useFixture(MockCmdRunnerPopenFixture())
+        self.useFixture(MockSomethingFixture(
+            linaro_media_create.hwpack, 'install_hwpack', mock_install_hwpack))
+        self.useFixture(MockSomethingFixture(
+            linaro_media_create.hwpack, 'run_local_atexit_funcs',
+            mock_run_local_atexit_functions))
+
+        force_yes = True
+        exception_caught = False
+        try:
+            install_hwpacks(
+                'chroot', '/tmp/dir', force_yes, 'hwp.tgz', 'hwp2.tgz')
+        except:
+            exception_caught = True
+        self.assertTrue(self.run_local_atexit_functions_called)
+        self.assertTrue(exception_caught)
+
+    def setUp(self):
+        super(TestInstallHWPack, self).setUp()
+        # Ensure the list of cleanup functions gets cleared to make sure tests
+        # don't interfere with one another.
+        def clear_atexits():
+            linaro_media_create.hwpack.local_atexit = []
+        self.addCleanup(clear_atexits)
+