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

diff --git a/daemon/mount.c b/daemon/mount.c
index 3abdabd..4e0ecdb 100644 (file)
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -188,29 +188,89 @@ do_mounts (void)
   return ret;
 }
 
   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)
 {
 int
 do_umount_all (void)
 {

-  char **mounts;
+  char *out, *err;

   int i, r;
   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;
     return -1;

+  }
+
+  free (err);

 
 

-  for (i = 0; mounts[i] != NULL; ++i) {
+  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);
     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);
   }
 
       return -1;
     }
     free (err);
   }
 

-  free_strings (mounts);
+  free_stringslen (mounts, size);
+
+  /* We've unmounted root now, so ... */
+  root_mounted = 0;
+

   return 0;
 }
   return 0;
 }

diff --git a/inspector/virt-inspector.pl b/inspector/virt-inspector.pl
index 2169431..b2983b3 100755 (executable)
--- a/inspector/virt-inspector.pl
+++ b/inspector/virt-inspector.pl
@@ -566,12 +566,6 @@ if ($output !~ /.*fish$/) {
        check_for_applications ($root_dev);
        check_for_kernels ($root_dev);
 
        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 ();
     }
 }
        $g->umount_all ();
     }
 }

diff --git a/src/generator.ml b/src/generator.ml
index be67807..ae640e2 100755 (executable)
--- a/src/generator.ml
+++ b/src/generator.ml
@@ -1074,6 +1074,20 @@ Some internal mounts are not shown.");
   ("umount_all", (RErr, []), 47, [FishAlias "unmount-all"],
    [InitBasicFS, TestOutputList (
       [["umount_all"];
   ("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",
    "\
        ["mounts"]], [])],
    "unmount all filesystems",
    "\

diff --git a/tests.c b/tests.c
index 2c30596..9a45cce 100644 (file)
--- a/tests.c
+++ b/tests.c
@@ -2744,6 +2744,126 @@ static int test_umount_all_0 (void)
   return 0;
 }
 
   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 */
 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);
   }
 
     exit (1);
   }
 

-  nr_tests = 90;
+  nr_tests = 91;

 
   test_num++;
   printf ("%3d/%3d test_set_e2uuid_0\n", test_num, nr_tests);
 
   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++;
     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");
   printf ("%3d/%3d test_mounts_0\n", test_num, nr_tests);
   if (test_mounts_0 () == -1) {
     printf ("test_mounts_0 FAILED\n");