Libguestfs - Jan 2020 - [PATCH v2 1/2] mltools, options: support --allow-discards when decrypting LUKS devices

---
 mltools/tools_utils-c.c | 8 ++++----
 mltools/tools_utils.ml  | 6 +++---
 mltools/tools_utils.mli | 8 ++++++--
 options/decrypt.c       | 5 +++--
 options/inspect.c       | 2 +-
 options/options.h       | 2 +-
 6 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c
index 6c43b8d..1dcebc4 100644
--- a/mltools/tools_utils-c.c
+++ b/mltools/tools_utils-c.c
@@ -36,7 +36,7 @@
 
 #include "options.h"
 
-extern value guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value
keysv);
+extern value guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value
keysv, value allowdiscards);
 extern value guestfs_int_mllib_set_echo_keys (value unitv);
 extern value guestfs_int_mllib_set_keys_from_stdin (value unitv);
 extern value guestfs_int_mllib_rfc3339_date_time_string (value unitv);
@@ -46,9 +46,9 @@ int echo_keys = 0;
 int keys_from_stdin = 0;
 
 value
-guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv)
+guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value keysv, value
allowdiscards)
 {
-  CAMLparam3 (gv, gpv, keysv);
+  CAMLparam4 (gv, gpv, keysv, allowdiscards);
   CAMLlocal2 (elemv, v);
   guestfs_h *g = (guestfs_h *) (intptr_t) Int64_val (gpv);
   struct key_store *ks = NULL;
@@ -86,7 +86,7 @@ guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value
keysv)
     keysv = Field (keysv, 1);
   }
 
-  inspect_do_decrypt (g, ks);
+  inspect_do_decrypt (g, ks, Int_val (allowdiscards));
 
   CAMLreturn (Val_unit);
 }
diff --git a/mltools/tools_utils.ml b/mltools/tools_utils.ml
index 1271802..cb94125 100644
--- a/mltools/tools_utils.ml
+++ b/mltools/tools_utils.ml
@@ -29,7 +29,7 @@ and key_store_key    | KeyString of string
   | KeyFileName of string
 
-external c_inspect_decrypt : Guestfs.t -> int64 -> (string *
key_store_key) list -> unit = "guestfs_int_mllib_inspect_decrypt"
+external c_inspect_decrypt : Guestfs.t -> int64 -> (string *
key_store_key) list -> bool -> unit =
"guestfs_int_mllib_inspect_decrypt"
 external c_set_echo_keys : unit -> unit =
"guestfs_int_mllib_set_echo_keys" "noalloc"
 external c_set_keys_from_stdin : unit -> unit =
"guestfs_int_mllib_set_keys_from_stdin" "noalloc"
 external c_rfc3339_date_time_string : unit -> string =
"guestfs_int_mllib_rfc3339_date_time_string"
@@ -650,7 +650,7 @@ let is_btrfs_subvolume g fs      if g#last_errno () =
Guestfs.Errno.errno_EINVAL then false
     else raise exn
 
-let inspect_decrypt g ks +let inspect_decrypt g ?(allow_discards = false) ks   
(* Turn the keys in the key_store into a simpler struct, so it is possible
    * to read it using the C API.
    *)
@@ -664,7 +664,7 @@ let inspect_decrypt g ks     * function.
    *)
   c_inspect_decrypt g#ocaml_handle (Guestfs.c_pointer g#ocaml_handle)
-    keys_as_list
+    keys_as_list allow_discards
 
 let with_timeout op timeout ?(sleep = 2) fn    let start_t = Unix.gettimeofday
() in
diff --git a/mltools/tools_utils.mli b/mltools/tools_utils.mli
index ab70f58..ac11a58 100644
--- a/mltools/tools_utils.mli
+++ b/mltools/tools_utils.mli
@@ -194,10 +194,14 @@ val inspect_mount_root_ro : Guestfs.guestfs -> string
-> unit
 val is_btrfs_subvolume : Guestfs.guestfs -> string -> bool
 (** Checks if a filesystem is a btrfs subvolume. *)
 
-val inspect_decrypt : Guestfs.guestfs -> key_store -> unit
+val inspect_decrypt : Guestfs.guestfs -> ?allow_discards:bool ->
key_store -> unit
 (** Simple implementation of decryption: look for any [crypto_LUKS]
     partitions and decrypt them, then rescan for VGs.  This only works
-    for Fedora whole-disk encryption. *)
+    for Fedora whole-disk encryption.
+
+    If [?allow_discards] is set, the underlying [crypto_LUKS] partitions
+    will be decrypted with the discard operation allowed, which allows
+    the partitions to be trimmed (and sparsified). Default is [false]. *)
 
 val with_timeout : string -> int -> ?sleep:int -> (unit -> 'a
option) -> 'a
 (** [with_timeout op timeout ?sleep fn] implements a timeout loop.
diff --git a/options/decrypt.c b/options/decrypt.c
index 683cf5e..7e24254 100644
--- a/options/decrypt.c
+++ b/options/decrypt.c
@@ -71,7 +71,7 @@ make_mapname (const char *device, char *mapname, size_t len)
  * encryption schemes.
  */
 void
-inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
+inspect_do_decrypt (guestfs_h *g, struct key_store *ks, bool allowdiscards)
 {
   CLEANUP_FREE_STRING_LIST char **partitions = guestfs_list_partitions (g);
   if (partitions == NULL)
@@ -101,7 +101,8 @@ inspect_do_decrypt (guestfs_h *g, struct key_store *ks)
          * is set?  This might break 'mount_ro'.
          */
         guestfs_push_error_handler (g, NULL, NULL);
-        r = guestfs_luks_open (g, partitions[i], keys[j], mapname);
+        r = guestfs_luks_open_opts (g, partitions[i], keys[j], mapname,
+                                    GUESTFS_LUKS_OPEN_OPTS_ALLOWDISCARDS,
allowdiscards, -1);
         guestfs_pop_error_handler (g);
         if (r == 0)
           goto opened;
diff --git a/options/inspect.c b/options/inspect.c
index 3de6d70..be69419 100644
--- a/options/inspect.c
+++ b/options/inspect.c
@@ -70,7 +70,7 @@ inspect_mount_handle (guestfs_h *g, struct key_store *ks)
   if (live)
     error (EXIT_FAILURE, 0, _("dont use --live and -i options
together"));
 
-  inspect_do_decrypt (g, ks);
+  inspect_do_decrypt (g, ks, false);
 
   char **roots = guestfs_inspect_os (g);
   if (roots == NULL)
diff --git a/options/options.h b/options/options.h
index 9b78302..2467804 100644
--- a/options/options.h
+++ b/options/options.h
@@ -137,7 +137,7 @@ struct key_store {
 extern void parse_config (void);
 
 /* in decrypt.c */
-extern void inspect_do_decrypt (guestfs_h *g, struct key_store *ks);
+extern void inspect_do_decrypt (guestfs_h *g, struct key_store *ks, bool
allowdiscards);
 
 /* in domain.c */
 extern int add_libvirt_drives (guestfs_h *g, const char *guest);
-- 
2.24.1

---
 daemon/listfs.ml          | 18 +++++++++++++++---
 daemon/luks.c             |  9 +++++----
 generator/actions_core.ml |  3 ++-
 gobject/Makefile.inc      |  2 ++
 inspector/inspector.c     |  2 +-
 sparsify/in_place.ml      |  2 +-
 6 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index bf4dca6d4..4f1af474a 100644
--- a/daemon/listfs.ml
+++ b/daemon/listfs.ml
@@ -19,6 +19,7 @@
 open Printf
 
 open Std_utils
+open Utils
 
 (* Enumerate block devices (including MD, LVM, LDM and partitions) and use
  * vfs-type to check for filesystems on devices.  Some block devices cannot
@@ -144,9 +145,20 @@ and check_with_vfs_type device    else if String.is_suffix
vfs_type "_member" then
     None
 
-  (* Ignore LUKS-encrypted partitions.  These are also containers, as above. *)
-  else if vfs_type = "crypto_LUKS" then
-    None
+  (* If a LUKS-encrypted partition had been opened, include the corresponding
+   * device mapper filesystem path. *)
+  else if vfs_type = "crypto_LUKS" then (
+    let out = command "lsblk" ["-n"; "-l";
"-o"; "NAME"; device] in
+      (* Example output: #lsblk -n -l -o NAME /dev/sda5
+       * sda5
+       * lukssda5
+       *)
+      match String.trimr @@ snd @@  String.split "\n" out with
+      | "" -> None
+      | part ->
+        let mnt = Mountable.of_path @@ "/dev/mapper/" ^ part in
+        Some [mnt, Blkid.vfs_type mnt]
+  )
 
   (* A single btrfs device can turn into many volumes. *)
   else if vfs_type = "btrfs" then (
diff --git a/daemon/luks.c b/daemon/luks.c
index d631cb100..306b2dcfb 100644
--- a/daemon/luks.c
+++ b/daemon/luks.c
@@ -83,7 +83,7 @@ remove_temp (char *tempfile)
 
 static int
 luks_open (const char *device, const char *key, const char *mapname,
-           int readonly)
+           int readonly, int allowdiscards)
 {
   /* Sanity check: /dev/mapper/mapname must not exist already.  Note
    * that the device-mapper control device (/dev/mapper/control) is
@@ -110,6 +110,7 @@ luks_open (const char *device, const char *key, const char
*mapname,
   ADD_ARG (argv, i, "-d");
   ADD_ARG (argv, i, tempfile);
   if (readonly) ADD_ARG (argv, i, "--readonly");
+  if (allowdiscards) ADD_ARG (argv, i, "--allow-discards");
   ADD_ARG (argv, i, "luksOpen");
   ADD_ARG (argv, i, device);
   ADD_ARG (argv, i, mapname);
@@ -130,15 +131,15 @@ luks_open (const char *device, const char *key, const char
*mapname,
 }
 
 int
-do_luks_open (const char *device, const char *key, const char *mapname)
+do_luks_open (const char *device, const char *key, const char *mapname, int
allowdiscards)
 {
-  return luks_open (device, key, mapname, 0);
+  return luks_open (device, key, mapname, 0, allowdiscards);
 }
 
 int
 do_luks_open_ro (const char *device, const char *key, const char *mapname)
 {
-  return luks_open (device, key, mapname, 1);
+  return luks_open (device, key, mapname, 1, 0);
 }
 
 int
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index cb7e8dcd0..662b63289 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -5631,7 +5631,8 @@ group scan." };
 
   { defaults with
     name = "luks_open"; added = (1, 5, 1);
-    style = RErr, [String (Device, "device"); String (Key,
"key"); String (PlainString, "mapname")], [];
+    style = RErr, [String (Device, "device"); String (Key,
"key"); String (PlainString, "mapname")], [OBool
"allowdiscards"];
+    once_had_no_optargs = true;
     optional = Some "luks";
     shortdesc = "open a LUKS-encrypted block device";
     longdesc = "\
diff --git a/gobject/Makefile.inc b/gobject/Makefile.inc
index 067f861a9..a7b856bee 100644
--- a/gobject/Makefile.inc
+++ b/gobject/Makefile.inc
@@ -86,6 +86,7 @@ guestfs_gobject_headers= \
   include/guestfs-gobject/optargs-is_fifo.h \
   include/guestfs-gobject/optargs-is_file.h \
   include/guestfs-gobject/optargs-is_socket.h \
+  include/guestfs-gobject/optargs-luks_open.h \
   include/guestfs-gobject/optargs-md_create.h \
   include/guestfs-gobject/optargs-mke2fs.h \
   include/guestfs-gobject/optargs-mkfs.h \
@@ -179,6 +180,7 @@ guestfs_gobject_sources= \
   src/optargs-is_fifo.c \
   src/optargs-is_file.c \
   src/optargs-is_socket.c \
+  src/optargs-luks_open.c \
   src/optargs-md_create.c \
   src/optargs-mke2fs.c \
   src/optargs-mkfs.c \
diff --git a/inspector/inspector.c b/inspector/inspector.c
index fa8e721ff..6ec3a51e7 100644
--- a/inspector/inspector.c
+++ b/inspector/inspector.c
@@ -298,7 +298,7 @@ main (int argc, char *argv[])
    * the -i option) because it can only handle a single root.  So we
    * use low-level APIs.
    */
-  inspect_do_decrypt (g, ks);
+  inspect_do_decrypt (g, ks, false);
 
   free_key_store (ks);
 
diff --git a/sparsify/in_place.ml b/sparsify/in_place.ml
index 7da83dafd..ade3c6843 100644
--- a/sparsify/in_place.ml
+++ b/sparsify/in_place.ml
@@ -62,7 +62,7 @@ let run disk format ignores zeroes ks      error ~exit_code:3
(f_"discard/trim is not supported");
 
   (* Decrypt the disks. *)
-  inspect_decrypt g ks;
+  inspect_decrypt g ~allow_discards:true ks;
 
   (* Discard non-ignored filesystems that we are able to mount, and
    * selected swap partitions.
-- 
2.24.1

On Monday, 27 January 2020 13:12:35 CET Jan Synacek
wrote:
> ---
>  mltools/tools_utils-c.c | 8 ++++----
>  mltools/tools_utils.ml  | 6 +++---
>  mltools/tools_utils.mli | 8 ++++++--
>  options/decrypt.c       | 5 +++--
>  options/inspect.c       | 2 +-
>  options/options.h       | 2 +-
>  6 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/mltools/tools_utils-c.c b/mltools/tools_utils-c.c
> index 6c43b8d..1dcebc4 100644
> --- a/mltools/tools_utils-c.c
> +++ b/mltools/tools_utils-c.c
> @@ -36,7 +36,7 @@
>  
>  #include "options.h"
>  
> -extern value guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value
keysv);
> +extern value guestfs_int_mllib_inspect_decrypt (value gv, value gpv, value
keysv, value allowdiscards);
We usually name arguments of type value in C implementations of OCaml
functions with a "v" suffix (see the other parameters, for example).
> -  inspect_do_decrypt (g, ks);
> +  inspect_do_decrypt (g, ks, Int_val (allowdiscards));
The parameter is 'bool', so this must be Bool_val() -- in practice it
is the same, however better use the right _val() function to avoid
unexpected surprises in newer OCaml versions.
>          guestfs_push_error_handler (g, NULL, NULL);
> -        r = guestfs_luks_open (g, partitions[i], keys[j], mapname);
> +        r = guestfs_luks_open_opts (g, partitions[i], keys[j], mapname,
> +                                    GUESTFS_LUKS_OPEN_OPTS_ALLOWDISCARDS,
allowdiscards, -1);
This is in the common submodule, while the new API parameter is in the
main libguestfs repository -- considering the common submodule is used
also by virt-v2v, we do not want to push the virt-v2v requirements to
the git version of libguestfs.

One way to get around that for now is to make the new API conditional;
see for example few lines above what was done for the luks_uuid.
guestfs.h provides #define's also for optional parameters of functions.

Thanks,
-- 
Pino Toscano

On Monday, 27 January 2020 13:12:36 CET Jan Synacek
wrote:
> ---
>  daemon/listfs.ml          | 18 +++++++++++++++---
>  daemon/luks.c             |  9 +++++----
>  generator/actions_core.ml |  3 ++-
>  gobject/Makefile.inc      |  2 ++
>  inspector/inspector.c     |  2 +-
>  sparsify/in_place.ml      |  2 +-
>  6 files changed, 26 insertions(+), 10 deletions(-)
The changes here ought to be split in different patches:
1) adding the optional parameter to luks_open
2) the adaptation of inspect_do_decrypt to the changes in common (which
   will be updated together)
3) the rest

The changes for #1 and #2 seems OK, so it will be easy to merge them.
> diff --git a/daemon/listfs.ml b/daemon/listfs.ml
> index bf4dca6d4..4f1af474a 100644
> --- a/daemon/listfs.ml
> +++ b/daemon/listfs.ml
> @@ -19,6 +19,7 @@
>  open Printf
>  
>  open Std_utils
> +open Utils
>  
>  (* Enumerate block devices (including MD, LVM, LDM and partitions) and use
>   * vfs-type to check for filesystems on devices.  Some block devices
cannot
> @@ -144,9 +145,20 @@ and check_with_vfs_type device >    else if
String.is_suffix vfs_type "_member" then
>      None
>  
> -  (* Ignore LUKS-encrypted partitions.  These are also containers, as
above. *)
> -  else if vfs_type = "crypto_LUKS" then
> -    None
> +  (* If a LUKS-encrypted partition had been opened, include the
corresponding
> +   * device mapper filesystem path. *)
> +  else if vfs_type = "crypto_LUKS" then (
> +    let out = command "lsblk" ["-n"; "-l";
"-o"; "NAME"; device] in
> +      (* Example output: #lsblk -n -l -o NAME /dev/sda5
> +       * sda5
> +       * lukssda5
> +       *)
> +      match String.trimr @@ snd @@  String.split "\n" out with
> +      | "" -> None
> +      | part ->
> +        let mnt = Mountable.of_path @@ "/dev/mapper/" ^ part in
> +        Some [mnt, Blkid.vfs_type mnt]
> +  )
I'm slightly skeptic about these changes: a LUKS device can contain
different things: a single filesystem, a device with a partition table
and filesystems, a LV, etc. Hence, we cannot assume the /dev/mapper
name will be a filesystem.
>    if (readonly) ADD_ARG (argv, i, "--readonly");
> +  if (allowdiscards) ADD_ARG (argv, i, "--allow-discards");
According to its release notes, cryptsetup added this option in 2.0.0:
it seems ~2y old, however non-that-old distros already have it.
So IMHO there is no need to add an existance check for this option,
which can be added later only in case we need to support older
versions.

Thanks,
-- 
Pino Toscano