Libguestfs - Mar 2012 - [PATCH 0/5] Fixes to resize2fs (RHBZ#755729, RHBZ#801640)

https://bugzilla.redhat.com/show_bug.cgi?id=755729

This bug reports that the error message printed by the resize2fs API
calls (which comes directly from the resize2fs command) says:

  Please run 'e2fsck -f /dev/vda1' first.

That command is not possible from guestfish (where it would be
'e2fsck-f' or 'e2fsck ... forceall:true').

Fixing that bug caused this bug:

https://bugzilla.redhat.com/show_bug.cgi?id=801640

By replacing the error message, we inadvertently replaced an important
(and different) error message:

  Please run 'e2fsck -fy /dev/vda1' to fix the filesystem
  after the aborted resize operation.

which occurs *after* a resize operation fails.

I proposed that we don't try editing error messages at all, even when
(as in 755729) this leads to some slightly inappropriate text being
printed in guestfish.

Trying to replace error messages is an open-ended task.  It risks
causing more bugs, and it obscures what the underlying utility is
trying to say.  Google exists, and people can use it to resolve
these problems.

This patch series starts by reverting the original fix.

Patch 4/5 causes the resize2fs* APIs to automatically run 'e2fsck -f'
when necessary, both simplifying the API and avoiding 755729.

Patch 5/5 documents the whole mess.

Rich.

From: "Richard W.M. Jones" <rjones at redhat.com>

This reverts commit 0eaf06e673833bc25673d5c3d2487fffae310285.
---
 daemon/ext2.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/daemon/ext2.c b/daemon/ext2.c
index 02cd68a..b1c6c02 100644
--- a/daemon/ext2.c
+++ b/daemon/ext2.c
@@ -255,10 +255,7 @@ do_resize2fs_M (const char *device)
 
   r = command (NULL, &err, prog, "-M", device, NULL);
   if (r == -1) {
-    if (strstr (err, "e2fsck -f"))
-      reply_with_error ("you need to run e2fsck with the correct and/or
forceall options first");
-    else
-      reply_with_error ("%s", err);
+    reply_with_error ("%s", err);
     free (err);
     return -1;
   }
-- 
1.7.9.1

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 generator/generator_actions.ml |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
index 1fb8e30..b859e5f 100644
--- a/generator/generator_actions.ml
+++ b/generator/generator_actions.ml
@@ -3551,7 +3551,7 @@ The returned list is sorted.
 
 See also C<guestfs_find0>.");
 
-  ("e2fsck_f", (RErr, [Device "device"], []), 108, [],
+  ("e2fsck_f", (RErr, [Device "device"], []), 108,
[DeprecatedBy "e2fsck"],
    [], (* lvresize tests this *)
    "check an ext2/ext3 filesystem",
    "\
-- 
1.7.9.1

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 daemon/ext2.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/daemon/ext2.c b/daemon/ext2.c
index b1c6c02..a427d7a 100644
--- a/daemon/ext2.c
+++ b/daemon/ext2.c
@@ -264,6 +264,7 @@ do_resize2fs_M (const char *device)
   return 0;
 }
 
+/* Takes optional arguments, consult optargs_bitmask. */
 int
 do_e2fsck (const char *device,
            int correct,
@@ -285,11 +286,10 @@ do_e2fsck (const char *device,
     forceall = 0;
 
   if (correct && forceall) {
-    reply_with_error("Only one of the options may be specified");
+    reply_with_error ("only one of the options 'correct',
'forceall' may be specified");
     return -1;
   }
 
-
   ADD_ARG (argv, i, prog);
   ADD_ARG (argv, i, "-f");
 
-- 
1.7.9.1

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 daemon/daemon.h                |    1 +
 daemon/ext2.c                  |   37 ++++++++++++++++++++++++++++++++++
 daemon/mount.c                 |   43 ++++++++++++++++++++++++++++++++++++++++
 generator/generator_actions.ml |   13 +----------
 resize/resize.ml               |    4 +--
 5 files changed, 84 insertions(+), 14 deletions(-)

diff --git a/daemon/daemon.h b/daemon/daemon.h
index c45a7fe..b515fe4 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -111,6 +111,7 @@ extern uint64_t optargs_bitmask;
 
 /*-- in mount.c --*/
 extern int is_root_mounted (void);
+extern int is_device_mounted (const char *device);
 
 /*-- in stubs.c (auto-generated) --*/
 extern void dispatch_incoming_message (XDR *);
diff --git a/daemon/ext2.c b/daemon/ext2.c
index a427d7a..09037fb 100644
--- a/daemon/ext2.c
+++ b/daemon/ext2.c
@@ -187,6 +187,34 @@ do_get_e2uuid (const char *device)
   return do_vfs_uuid (device);
 }
 
+/* If the filesystem is not mounted, run e2fsck -f on it unconditionally. */
+static int
+if_not_mounted_run_e2fsck (const char *device)
+{
+  char *err;
+  int r, mounted;
+  char prog[] = "e2fsck";
+
+  if (e2prog (prog) == -1)
+    return -1;
+
+  mounted = is_device_mounted (device);
+  if (mounted == -1)
+    return -1;
+
+  if (!mounted) {
+    r = command (NULL, &err, prog, "-fy", device, NULL);
+    if (r == -1) {
+      reply_with_error ("%s", err);
+      free (err);
+      return -1;
+    }
+    free (err);
+  }
+
+  return 0;
+}
+
 int
 do_resize2fs (const char *device)
 {
@@ -197,6 +225,9 @@ do_resize2fs (const char *device)
   if (e2prog (prog) == -1)
     return -1;
 
+  if (if_not_mounted_run_e2fsck (device) == -1)
+    return -1;
+
   r = command (NULL, &err, prog, device, NULL);
   if (r == -1) {
     reply_with_error ("%s", err);
@@ -229,6 +260,9 @@ do_resize2fs_size (const char *device, int64_t size)
   }
   size /= 1024;
 
+  if (if_not_mounted_run_e2fsck (device) == -1)
+    return -1;
+
   char buf[32];
   snprintf (buf, sizeof buf, "%" PRIi64 "K", size);
 
@@ -253,6 +287,9 @@ do_resize2fs_M (const char *device)
   if (e2prog (prog) == -1)
     return -1;
 
+  if (if_not_mounted_run_e2fsck (device) == -1)
+    return -1;
+
   r = command (NULL, &err, prog, "-M", device, NULL);
   if (r == -1) {
     reply_with_error ("%s", err);
diff --git a/daemon/mount.c b/daemon/mount.c
index 98b9488..0c393f7 100644
--- a/daemon/mount.c
+++ b/daemon/mount.c
@@ -65,6 +65,49 @@ is_root_mounted (void)
   return 0;
 }
 
+/* Return true iff 'device' is mounted under /sysroot.
+ *   1  : true, device is mounted
+ *   0  : false, device is not mounted
+ *   -1 : error, reply_with_* has been called
+ */
+int
+is_device_mounted (const char *device)
+{
+  FILE *fp;
+  struct mntent *m;
+  struct stat stat1, stat2;
+
+  if (stat (device, &stat1) == -1) {
+    reply_with_perror ("stat: %s", device);
+    return -1;
+  }
+
+  /* NB: Eventually we should aim to parse /proc/self/mountinfo, but
+   * that requires custom parsing code.
+   */
+  fp = setmntent ("/proc/mounts", "r");
+  if (fp == NULL) {
+    perror ("/proc/mounts");
+    exit (EXIT_FAILURE);
+  }
+
+  while ((m = getmntent (fp)) != NULL) {
+    if ((sysroot_len > 0 && STREQ (m->mnt_dir, sysroot)) ||
+        (STRPREFIX (m->mnt_dir, sysroot) &&
m->mnt_dir[sysroot_len] == '/')) {
+      if (stat (m->mnt_fsname, &stat2) == 0) {
+        if (stat1.st_rdev == stat2.st_rdev) {
+          /* found it */
+          endmntent (fp);
+          return 1;
+        }
+      }
+    }
+  }
+
+  endmntent (fp);
+  return 0;
+}
+
 /* The "simple mount" call offers no complex options, you can just
  * mount a device on a mountpoint.  The variations like mount_ro,
  * mount_options and mount_vfs let you set progressively more things.
diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
index b859e5f..3c87d6d 100644
--- a/generator/generator_actions.ml
+++ b/generator/generator_actions.ml
@@ -3502,13 +3502,7 @@ is lost.");
    "resize an ext2, ext3 or ext4 filesystem",
    "\
 This resizes an ext2, ext3 or ext4 filesystem to match the size of
-the underlying device.
-
-I<Note:> It is sometimes required that you run C<guestfs_e2fsck_f>
-on the C<device> before calling this command.  For unknown reasons
-C<resize2fs> sometimes gives an error about this and sometimes not.
-In any case, it is always safe to call C<guestfs_e2fsck_f> before
-calling this function.");
+the underlying device.");
 
   ("find", (RStringList "names", [Pathname
"directory"], []), 107, [ProtocolLimitWarning],
    [InitBasicFS, Always, TestOutputList (
@@ -3557,10 +3551,7 @@ See also C<guestfs_find0>.");
    "\
 This runs C<e2fsck -p -f device>, ie. runs the ext2/ext3
 filesystem checker on C<device>, noninteractively (I<-p>),
-even if the filesystem appears to be clean (I<-f>).
-
-This command is only needed because of C<guestfs_resize2fs>
-(q.v.).  Normally you should use C<guestfs_fsck>.");
+even if the filesystem appears to be clean (I<-f>).");
 
   ("sleep", (RErr, [Int "secs"], []), 109, [],
    [InitNone, Always, TestRun (
diff --git a/resize/resize.ml b/resize/resize.ml
index 675a6e1..407d80f 100644
--- a/resize/resize.ml
+++ b/resize/resize.ml
@@ -1120,9 +1120,7 @@ let ()      (* Helper function to expand partition or LV
content. *)
     let do_expand_content target = function
       | PVResize -> g#pvresize target
-      | Resize2fs ->
-          g#e2fsck_f target;
-          g#resize2fs target
+      | Resize2fs -> g#resize2fs target
       | NTFSResize -> g#ntfsresize_opts ~force:ntfsresize_force target
       | BtrfsFilesystemResize ->
           (* Complicated ...  Btrfs forces us to mount the filesystem
-- 
1.7.9.1

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 generator/generator_actions.ml |   12 +++++++++---
 src/guestfs.pod                |   26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/generator/generator_actions.ml b/generator/generator_actions.ml
index 3c87d6d..4c6e704 100644
--- a/generator/generator_actions.ml
+++ b/generator/generator_actions.ml
@@ -3502,7 +3502,9 @@ is lost.");
    "resize an ext2, ext3 or ext4 filesystem",
    "\
 This resizes an ext2, ext3 or ext4 filesystem to match the size of
-the underlying device.");
+the underlying device.
+
+See also L<guestfs(3)/RESIZE2FS ERRORS>.");
 
   ("find", (RStringList "names", [Pathname
"directory"], []), 107, [ProtocolLimitWarning],
    [InitBasicFS, Always, TestOutputList (
@@ -5655,7 +5657,9 @@ See also C<guestfs_pread>,
C<guestfs_pwrite_device>.");
    "resize an ext2, ext3 or ext4 filesystem (with size)",
    "\
 This command is the same as C<guestfs_resize2fs> except that it
-allows you to specify the new size (in bytes) explicitly.");
+allows you to specify the new size (in bytes) explicitly.
+
+See also L<guestfs(3)/RESIZE2FS ERRORS>.");
 
   ("pvresize_size", (RErr, [Device "device"; Int64
"size"], []), 249, [Optional "lvm2"],
    [],
@@ -6157,7 +6161,9 @@ to the C<resize2fs> command.
 To get the resulting size of the filesystem you should call
 C<guestfs_tune2fs_l> and read the C<Block size> and C<Block
count>
 values.  These two numbers, multiplied together, give the
-resulting size of the minimal filesystem in bytes.");
+resulting size of the minimal filesystem in bytes.
+
+See also L<guestfs(3)/RESIZE2FS ERRORS>.");
 
   ("internal_autosync", (RErr, [], []), 282, [NotInFish; NotInDocs],
    [],
diff --git a/src/guestfs.pod b/src/guestfs.pod
index 3664ccf..531d123 100644
--- a/src/guestfs.pod
+++ b/src/guestfs.pod
@@ -703,6 +703,32 @@ ntfs-3g filesystems (using L</guestfs_getxattr>). 
See:
 
 L<http://www.tuxera.com/community/ntfs-3g-advanced/extended-attributes/>
 
+=head2 RESIZE2FS ERRORS
+
+The L</guestfs_resize2fs>, L</guestfs_resize2fs_size> and
+L</guestfs_resize2fs_M> calls are used to resize ext2/3/4 filesystems.
+
+The underlying program (L<resize2fs(8)>) requires that the filesystem
+is clean and recently fsck'd before you can resize it.  Also, if the
+resize operation fails for some reason, then you had to call fsck the
+filesystem again to fix it.
+
+In libguestfs C<lt> 1.17.14, you usually had to call
+L</guestfs_e2fsck_f> before the resize.  However, in C<ge> 1.17.14,
+L<e2fsck(8)> is called automatically before the resize, so you no
+longer need to do this.
+
+The L<resize2fs(8)> program can still fail, in which case it prints an
+error message similar to:
+
+ Please run 'e2fsck -fy <device>' to fix the filesystem
+ after the aborted resize operation.
+
+You can do this by calling L</guestfs_e2fsck> with the C<forceall>
+option.  However in the context of disk images, it is usually better
+to avoid this situation, eg. by rolling back to an earlier snapshot,
+or by copying and resizing and on failure going back to the original.
+
 =head2 USING LIBGUESTFS WITH OTHER PROGRAMMING LANGUAGES
 
 Although we don't want to discourage you from using the C API, we will
-- 
1.7.9.1