Fix umount_all command so it unmounts filesystems in the correct order.

daemon/mount.c

@@ -188,29 +188,89 @@ do_mounts (void)
   return ret;
 }
 
-/* Only unmount stuff under /sysroot */
+/* Unmount everything mounted under /sysroot.
+ *
+ * We have to unmount in the correct order, so we sort the paths by
+ * longest first to ensure that child paths are unmounted by parent
+ * paths.
+ *
+ * This call is more important than it appears at first, because it
+ * is widely used by both test and production code in order to
+ * get back to a known state (nothing mounted, everything synchronized).
+ */
+static int
+compare_longest_first (const void *vp1, const void *vp2)
+{
+  char * const *p1 = (char * const *) vp1;
+  char * const *p2 = (char * const *) vp2;
+  int n1 = strlen (*p1);
+  int n2 = strlen (*p2);
+  return n2 - n1;
+}
+
 int
 do_umount_all (void)
 {
-  char **mounts;
+  char *out, *err;
   int i, r;
-  char *err;
+  char **mounts = NULL;
+  int size = 0, alloc = 0;
+  char *p, *p2, *p3, *pend;
 
-  mounts = do_mounts ();
-  if (mounts == NULL)		/* do_mounts has already replied */
+  r = command (&out, &err, "mount", NULL);
+  if (r == -1) {
+    reply_with_error ("mount: %s", err);
+    free (out);
+    free (err);
     return -1;
+  }
 
-  for (i = 0; mounts[i] != NULL; ++i) {
+  free (err);
+
+  p = out;
+  while (p) {
+    pend = strchr (p, '\n');
+    if (pend) {
+      *pend = '\0';
+      pend++;
+    }
+
+    /* Lines have the format:
+     *   /dev/foo on /mountpoint type ...
+     */
+    p2 = strstr (p, " on /sysroot");
+    if (p2 != NULL) {
+      p2 += 4;
+      p3 = p2 + strcspn (p2, " ");
+      *p3 = '\0';
+      if (add_string (&mounts, &size, &alloc, p2) == -1) {
+	free (out);
+	return -1;
+      }
+    }
+
+    p = pend;
+  }
+  free (out);
+
+  qsort (mounts, size, sizeof (char *), compare_longest_first);
+
+  /* Unmount them. */
+  for (i = 0; i < size; ++i) {
     r = command (NULL, &err, "umount", mounts[i], NULL);
     if (r == -1) {
       reply_with_error ("umount: %s: %s", mounts[i], err);
       free (err);
-      free_strings (mounts);
+      free_stringslen (mounts, size);
       return -1;
     }
     free (err);
   }
 
-  free_strings (mounts);
+  free_stringslen (mounts, size);
+
+  /* We've unmounted root now, so ... */
+  root_mounted = 0;
+
   return 0;
 }

inspector/virt-inspector.pl

@@ -566,12 +566,6 @@ if ($output !~ /.*fish$/) {
 	check_for_applications ($root_dev);
 	check_for_kernels ($root_dev);
 
-	# umount_all in libguestfs is buggy - it doesn't unmount
-	# filesystems in the correct order.  So let's unmount them
-	# in reverse first before calling umount_all as a last resort.
-	foreach (sort { $b cmp $a } keys %$mounts) {
-	    eval "\$g->umount ('$_')";
-	}
 	$g->umount_all ();
     }
 }

src/generator.ml

@@ -1074,6 +1074,20 @@ Some internal mounts are not shown.");
   ("umount_all", (RErr, []), 47, [FishAlias "unmount-all"],
    [InitBasicFS, TestOutputList (
       [["umount_all"];
+       ["mounts"]], []);
+    (* check that umount_all can unmount nested mounts correctly: *)
+    InitEmpty, TestOutputList (
+      [["sfdisk"; "/dev/sda"; "0"; "0"; "0"; ",10 ,20 ,"];
+       ["mkfs"; "ext2"; "/dev/sda1"];
+       ["mkfs"; "ext2"; "/dev/sda2"];
+       ["mkfs"; "ext2"; "/dev/sda3"];
+       ["mount"; "/dev/sda1"; "/"];
+       ["mkdir"; "/mp1"];
+       ["mount"; "/dev/sda2"; "/mp1"];
+       ["mkdir"; "/mp1/mp2"];
+       ["mount"; "/dev/sda3"; "/mp1/mp2"];
+       ["mkdir"; "/mp1/mp2/mp3"];
+       ["umount_all"];
        ["mounts"]], [])],
    "unmount all filesystems",
    "\

tests.c

@@ -2744,6 +2744,126 @@ static int test_umount_all_0 (void)
   return 0;
 }
 
+static int test_umount_all_1 (void)
+{
+  /* InitEmpty for umount_all (1) */
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_umount_all (g);
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_lvm_remove_all (g);
+    if (r == -1)
+      return -1;
+  }
+  /* TestOutputList for umount_all (1) */
+  {
+    char *lines[] = {
+      ",10",
+      ",20",
+      ",",
+      NULL
+    };
+    int r;
+    suppress_error = 0;
+    r = guestfs_sfdisk (g, "/dev/sda", 0, 0, 0, lines);
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkfs (g, "ext2", "/dev/sda1");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkfs (g, "ext2", "/dev/sda2");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkfs (g, "ext2", "/dev/sda3");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mount (g, "/dev/sda1", "/");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkdir (g, "/mp1");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mount (g, "/dev/sda2", "/mp1");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkdir (g, "/mp1/mp2");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mount (g, "/dev/sda3", "/mp1/mp2");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_mkdir (g, "/mp1/mp2/mp3");
+    if (r == -1)
+      return -1;
+  }
+  {
+    int r;
+    suppress_error = 0;
+    r = guestfs_umount_all (g);
+    if (r == -1)
+      return -1;
+  }
+  {
+    char **r;
+    int i;
+    suppress_error = 0;
+    r = guestfs_mounts (g);
+    if (r == NULL)
+      return -1;
+    if (r[0] != NULL) {
+      fprintf (stderr, "test_umount_all_1: extra elements returned from command\n");
+      print_strings (r);
+      return -1;
+    }
+    for (i = 0; r[i] != NULL; ++i)
+      free (r[i]);
+    free (r);
+  }
+  return 0;
+}
+
 static int test_mounts_0 (void)
 {
   /* InitBasicFS for mounts (0): create ext2 on /dev/sda1 */
@@ -6393,7 +6513,7 @@ int main (int argc, char *argv[])
     exit (1);
   }
 
-  nr_tests = 90;
+  nr_tests = 91;
 
   test_num++;
   printf ("%3d/%3d test_set_e2uuid_0\n", test_num, nr_tests);
@@ -6654,6 +6774,12 @@ int main (int argc, char *argv[])
     failed++;
   }
   test_num++;
+  printf ("%3d/%3d test_umount_all_1\n", test_num, nr_tests);
+  if (test_umount_all_1 () == -1) {
+    printf ("test_umount_all_1 FAILED\n");
+    failed++;
+  }
+  test_num++;
   printf ("%3d/%3d test_mounts_0\n", test_num, nr_tests);
   if (test_mounts_0 () == -1) {
     printf ("test_mounts_0 FAILED\n");