Libguestfs - Aug 2016 - Re: [PATCH 2/4] daemon: fstrim: Turn "discard operation is not supported" into ENOTSUP.

Fix for
https://bugzilla.redhat.com/show_bug.cgi?id=1364347

This replaces the indirect exception catching from
commit 931aec5b8846ce63b0e2b6beba5861a019b5f694 with a direct feature
test.

Fixes commit 931aec5b8846ce63b0e2b6beba5861a019b5f694.
---
 sparsify/in_place.ml | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml
index 389ea44..5cd03ef 100644
--- a/sparsify/in_place.ml
+++ b/sparsify/in_place.ml
@@ -29,7 +29,7 @@ open Cmdline
 
 module G = Guestfs
 
-let rec run disk format ignores machine_readable zeroes +let run disk format
ignores machine_readable zeroes    (* Connect to libguestfs. *)
   let g = open_guestfs () in
 
@@ -40,17 +40,6 @@ let rec run disk format ignores machine_readable zeroes   
Sys.set_signal Sys.sigquit (Sys.Signal_handle set_quit);
   g#set_pgroup true;
 
-  try
-    perform g disk format ignores machine_readable zeroes quit
-  with
-    G.Error msg as exn ->
-      if g#last_errno () = G.Errno.errno_ENOTSUP then (
-        (* for exit code 3, see man page *)
-        error ~exit_code:3 (f_"discard/trim is not supported: %s")
msg;
-      )
-      else raise exn
-
-and perform g disk format ignores machine_readable zeroes quit    (* XXX
Current limitation of the API.  Can remove this hunk in future. *)
   let format      match format with
@@ -62,6 +51,12 @@ and perform g disk format ignores machine_readable zeroes
quit    if not (quiet ()) then Progress.set_up_progress_bar ~machine_readable g;
   g#launch ();
 
+  (* If discard is not supported in the appliance, we must return exit
+   * code 3.  See the man page.
+   *)
+  if not (g#feature_available [|"fstrim"|]) then
+    error ~exit_code:3 (f_"discard/trim is not supported");
+
   (* Discard non-ignored filesystems that we are able to mount, and
    * selected swap partitions.
    *)
-- 
2.7.4

Because we run the external fstrim command we don't have access to the
kernel errno when it fails.  However in the case where it prints this
specific error message, turn that into errno ENOTSUP.
---
 daemon/fstrim.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/daemon/fstrim.c b/daemon/fstrim.c
index 1ad3630..527acfd 100644
--- a/daemon/fstrim.c
+++ b/daemon/fstrim.c
@@ -105,7 +105,13 @@ do_fstrim (const char *path,
 
   r = commandv (&out, &err, argv);
   if (r == -1) {
-    reply_with_error ("%s", err);
+    /* If the error is about the kernel operation not being supported
+     * for this filesystem type, then return errno ENOTSUP here.
+     */
+    if (strstr (err, "discard operation is not supported"))
+      reply_with_error_errno (ENOTSUP, "%s", err);
+    else
+      reply_with_error ("%s", err);
     return -1;
   }
 
-- 
2.7.4

Thanks:  Xianghua Chen
---
 sparsify/in_place.ml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml
index 5cd03ef..e2ee9d9 100644
--- a/sparsify/in_place.ml
+++ b/sparsify/in_place.ml
@@ -89,7 +89,14 @@ let run disk format ignores machine_readable zeroes          
if mounted then (
               message (f_"Trimming %s") fs;
 
-              g#fstrim "/"
+              try g#fstrim "/"
+              with G.Error msg as exn ->
+                if g#last_errno () = G.Errno.errno_ENOTSUP then (
+                  let vfs_type = try g#vfs_type fs with _ ->
"unknown" in
+                  warning (f_"fstrim operation is not supported on %s
(%s).  Suppress this warning using '--ignore %s', or use copying mode
instead.")
+                          fs vfs_type fs
+                )
+                else raise exn
             ) else (
               let is_linux_x86_swap                  (* Look for the signature
for Linux swap on i386.
-- 
2.7.4

Add a regression test for the previous commit.
---
 sparsify/Makefile.am                               |  4 +-
 ...st-virt-sparsify-in-place-fstrim-unsupported.sh | 84 ++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletion(-)
 create mode 100755 sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh

diff --git a/sparsify/Makefile.am b/sparsify/Makefile.am
index f8d2641..5e42f0b 100644
--- a/sparsify/Makefile.am
+++ b/sparsify/Makefile.am
@@ -22,6 +22,7 @@ EXTRA_DIST = \
 	test-virt-sparsify.sh \
 	test-virt-sparsify-docs.sh \
 	test-virt-sparsify-in-place.sh \
+	test-virt-sparsify-in-place-fstrim-unsupported.sh \
 	virt-sparsify.pod
 
 CLEANFILES = *~ *.annot *.cmi *.cmo *.cmx *.cmxa *.o virt-sparsify
@@ -126,7 +127,8 @@ TESTS = \
 if ENABLE_APPLIANCE
 TESTS += \
 	test-virt-sparsify.sh \
-	test-virt-sparsify-in-place.sh
+	test-virt-sparsify-in-place.sh \
+	test-virt-sparsify-in-place-fstrim-unsupported.sh
 endif ENABLE_APPLIANCE
 
 check-valgrind:
diff --git a/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh
b/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh
new file mode 100755
index 0000000..d6a300d
--- /dev/null
+++ b/sparsify/test-virt-sparsify-in-place-fstrim-unsupported.sh
@@ -0,0 +1,84 @@
+#!/bin/bash -
+# libguestfs virt-sparsify --in-place test script
+# Copyright (C) 2011-2016 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that we do the right thing for filesystems where the fstrim
+# operation is not supported.
+#
+# https://bugzilla.redhat.com/show_bug.cgi?id=1364347
+#
+# This test assumes that the kernel vfat driver does not support
+# fstrim.  It might become supported in a future kernel version in
+# which case we could use a different filesystem for this test, or
+# delete the test if we are confident that all common filesystems are
+# supported.
+#
+# The reason why vfat is significant is because UEFI guests use it.
+
+export LANG=C
+set -e
+set -x
+
+if [ -n "$SKIP_TEST_VIRT_SPARSIFY_IN_PLACE_FSTRIM_UNSUPPORTED_SH" ];
then
+    echo "$0: skipping test (environment variable set)"
+    exit 77
+fi
+
+if [ "$(guestfish get-backend)" = "uml" ]; then
+    echo "$0: skipping test because uml backend does not support
discard"
+    exit 77
+fi
+
+img=test-virt-sparsify-in-place-fstrim-unsupported.img
+log=test-virt-sparsify-in-place-fstrim-unsupported.log
+rm -f $img $log
+
+# Create a test filesystem with a single vfat filesystem.
+guestfish -N $img=fs:vfat exit
+
+# This should warn.
+virt-sparsify --in-place $img |& tee $log
+
+# Check the warning was emitted.
+grep "warning:.*fstrim" $log
+
+# This should ignore the filesystem and not warn.
+virt-sparsify --in-place --ignore /dev/sda1 $img |& tee $log
+
+if grep "warning:.*fstrim.*not supported" $log; then
+    echo "$0: filesystem /dev/sda1 was not ignored"
+    exit 1
+fi
+
+# Create a test filesystem with vfat and ext4 filesystems.
+guestfish -N $img=bootroot:vfat:ext4 exit
+
+# This should warn.
+virt-sparsify --in-place $img |& tee $log
+
+# Check the warning was emitted.
+grep "warning:.*fstrim" $log
+
+# This should ignore the filesystem and not warn.
+virt-sparsify --in-place --ignore /dev/sda1 $img |& tee $log
+
+if grep "warning:.*fstrim.*not supported" $log; then
+    echo "$0: filesystem /dev/sda1 was not ignored"
+    exit 1
+fi
+
+rm $img $log
-- 
2.7.4

On Friday, 5 August 2016 12:22:45 CEST Richard W.M. Jones
wrote:
> Because we run the external fstrim command we don't have access to the
> kernel errno when it fails.  However in the case where it prints this
> specific error message, turn that into errno ENOTSUP.
> ---
Can you please note this behaviour in the documentation of the fstrim
API?

Thanks,
-- 
Pino Toscano

On Friday, 5 August 2016 12:22:43 CEST Richard W.M. Jones
wrote:
> Fix for
> https://bugzilla.redhat.com/show_bug.cgi?id=1364347
The series mostly LGTM -- left only one note for patch #2 (so it's a
documented behaviour).

Thanks,
-- 
Pino Toscano