two diskimage edge case fixes

* DiskImage: Fix rsync crash when a mount point does not exist in the
  chroot.
* Fix bug in unmountBelow that caused unmounting of nested mounts to
  fail.

This commit was sponsored by Jack Hill on Patreon.

4 files changed, 39 insertions, 8 deletions

diff --git a/debian/changelog b/debian/changelog
index bfb884ce..3da0622f 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,10 @@ propellor (5.2.0) UNRELEASED; urgency=medium
   * DiskImage: Use safeAlignment. It didn't seem worth making the
     alignment configurable here.
   * Fixed rounding bug in Parted.calcPartTable.
+  * DiskImage: Fix rsync crash when a mount point does not exist in the
+    chroot.
+  * Fix bug in unmountBelow that caused unmounting of nested mounts to
+    fail.
 
  -- Joey Hess <id@joeyh.name>  Wed, 29 Nov 2017 11:45:08 -0400
 
diff --git a/doc/forum/imageBuiltFor_mount_points_not_automatically_created/comment_2_f90109eb016065586225fc59674c431c._comment b/doc/forum/imageBuiltFor_mount_points_not_automatically_created/comment_2_f90109eb016065586225fc59674c431c._comment
new file mode 100644
index 00000000..d8655a59
--- /dev/null
+++ b/doc/forum/imageBuiltFor_mount_points_not_automatically_created/comment_2_f90109eb016065586225fc59674c431c._comment
@@ -0,0 +1,20 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 2"""
+ date="2017-12-20T21:36:36Z"
+ content="""
+AFAICS, DiskImage.imageFinalized creates any mount points that were not
+rsynced over from the chroot. But, I guess that partitionsPopulated is
+expecting the mount point to exist in order to rsync its content over,
+and if nothing created /boot/efi that would explain the rsync error.
+I've added a check to prevent that problem.
+
+As for the unmount problem, it's a problem with the order
+it traverses the mount points for unmounting. That is using
+"unmountBelow" with the directory where the disk image partitions
+are loop mounted. If that unmounts `boot` first, it will implicitly
+unmount `boot/efi` (due to --lazy) and then will fail when it
+tries to explicitly unmount it. Added sorting that should fix that.
+
+Let me know how that goes, I've not tested it with your config.
+"""]]
diff --git a/src/Propellor/Property/DiskImage.hs b/src/Propellor/Property/DiskImage.hs
index 79865db4..c9998d3c 100644
--- a/src/Propellor/Property/DiskImage.hs
+++ b/src/Propellor/Property/DiskImage.hs
@@ -274,13 +274,18 @@ partitionsPopulated chrootdir mnts mntopts devs = property' desc $ \w ->
 	desc = "partitions populated from " ++ chrootdir
 
 	go _ Nothing _ _ = noChange
-	go w (Just mnt) mntopt loopdev = withTmpDir "mnt" $ \tmpdir -> bracket
-		(liftIO $ mount "auto" (partitionLoopDev loopdev) tmpdir mntopt)
-		(const $ liftIO $ umountLazy tmpdir)
-		$ \ismounted -> if ismounted
-			then ensureProperty w $
-				syncDirFiltered (filtersfor mnt) (chrootdir ++ mnt) tmpdir
-			else return FailedChange
+	go w (Just mnt) mntopt loopdev = ifM (liftIO $ doesDirectoryExist srcdir) $
+		( withTmpDir "mnt" $ \tmpdir -> bracket
+			(liftIO $ mount "auto" (partitionLoopDev loopdev) tmpdir mntopt)
+			(const $ liftIO $ umountLazy tmpdir)
+			$ \ismounted -> if ismounted
+				then ensureProperty w $
+					syncDirFiltered (filtersfor mnt) srcdir tmpdir
+				else return FailedChange
+		, return NoChange
+		)
+	  where
+		srcdir = chrootdir ++ mnt
 
 	filtersfor mnt =
 		let childmnts = map (drop (length (dropTrailingPathSeparator mnt))) $
diff --git a/src/Propellor/Property/Mount.hs b/src/Propellor/Property/Mount.hs
index c047161d..e8f3f092 100644
--- a/src/Propellor/Property/Mount.hs
+++ b/src/Propellor/Property/Mount.hs
@@ -149,4 +149,6 @@ umountLazy mnt =
 unmountBelow :: FilePath -> IO ()
 unmountBelow d = do
 	submnts <- mountPointsBelow d
-	forM_ submnts umountLazy
+	-- sort so sub-mounts are unmounted before the mount point
+	-- containing them
+	forM_ (sort submnts) umountLazy