From: Richard Jones Date: Thu, 30 Apr 2009 16:23:57 +0000 (+0100) Subject: Fix umount_all command so it unmounts filesystems in the correct order. X-Git-Tag: 1.0.16~11 X-Git-Url: http://git.annexia.org/?p=libguestfs.git;a=commitdiff_plain;h=e4733575efff31742444b180cdcfbc2504b144c8 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 --- a/daemon/mount.c +++ b/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; + } + + 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); - 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; } diff --git a/inspector/virt-inspector.pl b/inspector/virt-inspector.pl index 2169431..b2983b3 100755 --- 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); - # 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 (); } } diff --git a/src/generator.ml b/src/generator.ml index be67807..ae640e2 100755 --- 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"]; + ["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", "\ diff --git a/tests.c b/tests.c index 2c30596..9a45cce 100644 --- a/tests.c +++ b/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");