Fix #166: pmbootstrap shutdown: umount deep folder levels first (#274)

* Refactored `umount_all` to get the list of folders to be umounted from
  `umount_all_list`, so we can test that function in a test case.
* Adjusted `umount_all_list` to return the deep folder levels first
* Added a testcase for that
* Remove redundant calls to `umount_all()` (which were from a time before
  `pmbootstrap` was released, in which failing commands did not cause
  `pmbootstrap` to raise an exception)

pmb/chroot/shutdown.py

@@ -56,8 +56,6 @@ def shutdown(args, only_install_related=False):
     pmb.chroot.distccd.stop(args)
 
     # Umount installation-related paths (order is important!)
-    pmb.helpers.mount.umount_all(args, args.work +
-                                 "/chroot_native/mnt/install/boot")
     pmb.helpers.mount.umount_all(args, args.work +
                                  "/chroot_native/mnt/install")
     shutdown_cryptsetup_device(args, "pm_crypt")
@@ -73,7 +71,6 @@ def shutdown(args, only_install_related=False):
     if not only_install_related:
         # Clean up the rest
         pmb.helpers.mount.umount_all(args, args.work)
-        pmb.helpers.mount.umount_all(args, args.work)
         arch = args.deviceinfo["arch"]
         if pmb.parse.arch.cpu_emulation_required(args, arch):
             pmb.chroot.binfmt.unregister(args, arch)

pmb/helpers/mount.py

@@ -60,10 +60,12 @@ def bind(args, source, destination, create_folders=True):
     if not ismount(destination):
         raise RuntimeError("Mount failed: " + source + " -> " + destination)
 
-# Mount a blockdevice
-
 
 def bind_blockdevice(args, source, destination):
+    """
+    Mount a blockdevice with the --bind option, and create the destination
+    file, if necessary.
+    """
     # Skip existing mountpoint
     if ismount(destination):
         return
@@ -77,16 +79,31 @@ def bind_blockdevice(args, source, destination):
                                 destination])
 
 
+def umount_all_list(prefix, source="/proc/mounts"):
+    """
+    Parses `/proc/mounts` for all folders beginning with a prefix.
+    :source: can be changed for testcases
+    :returns: a list of folders, that need to be umounted
+    """
+    ret = []
+    prefix = os.path.abspath(prefix)
+    with open(source, "r") as handle:
+        for line in handle:
+            words = line.split()
+            if len(words) < 2:
+                raise RuntimeError("Failed to parse line in " + source + ": " +
+                                   line)
+            if words[1].startswith(prefix):
+                ret.append(words[1])
+    ret.sort(reverse=True)
+    return ret
+
+
 def umount_all(args, folder):
     """
     Umount all folders, that are mounted inside a given folder.
     """
-    folder = os.path.abspath(folder)
-    with open("/proc/mounts", "r") as handle:
-        for line in handle:
-            words = line.split()
-            if len(words) < 2 or not words[1].startswith(folder):
-                continue
-            pmb.helpers.run.root(args, ["umount", words[1]])
-            if ismount(words[1]):
-                raise RuntimeError("Failed to umount: " + words[1])
+    for mountpoint in umount_all_list(folder):
+        pmb.helpers.run.root(args, ["umount", mountpoint])
+        if ismount(mountpoint):
+            raise RuntimeError("Failed to umount: " + mountpoint)

test/test_mount.py

@@ -0,0 +1,45 @@
+"""
+Copyright 2017 Oliver Smith
+
+This file is part of pmbootstrap.
+
+pmbootstrap 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 of the License, or
+(at your option) any later version.
+
+pmbootstrap 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 pmbootstrap.  If not, see <http://www.gnu.org/licenses/>.
+"""
+import os
+import sys
+
+# Import from parent directory
+sys.path.append(os.path.abspath(
+    os.path.join(os.path.dirname(__file__) + "/..")))
+import pmb.helpers.mount
+
+
+def test_umount_all_list(tmpdir):
+    # Write fake mounts file
+    fake_mounts = str(tmpdir + "/mounts")
+    with open(fake_mounts, "w") as handle:
+        handle.write("source /test/var/cache\n")
+        handle.write("source /test/home/user/packages\n")
+        handle.write("source /test\n")
+        handle.write("source /test/proc\n")
+
+    ret = pmb.helpers.mount.umount_all_list("/no/match", fake_mounts)
+    assert ret == []
+
+    ret = pmb.helpers.mount.umount_all_list("/test/var/cache", fake_mounts)
+    assert ret == ["/test/var/cache"]
+
+    ret = pmb.helpers.mount.umount_all_list("/test", fake_mounts)
+    assert ret == ["/test/var/cache", "/test/proc", "/test/home/user/packages",
+                   "/test"]