Libguestfs - Jan 2020 - Re: [PATCH 1/1] sparsify: support LUKS-encrypted partitions

From: Jan Synacek <jan.synacek@redhat.com>

---
 daemon/listfs.ml          | 19 ++++++++++++++++---
 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, 27 insertions(+), 10 deletions(-)

diff --git a/daemon/listfs.ml b/daemon/listfs.ml
index bf4dca6d4..a618513e8 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
@@ -30,6 +31,7 @@ let rec list_filesystems ()  
   (* Devices. *)
   let devices = Devsparts.list_devices () in
+
   let devices = List.filter is_not_partitioned_device devices in
   let ret = List.filter_map check_with_vfs_type devices in
 
@@ -144,9 +146,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..db322a19a 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, 0);
 
   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 Wed, Jan 22, 2020 at 10:17 AM Jan Synacek <jsynacek@redhat.com> wrote:
> From: Jan Synacek <jan.synacek@redhat.com>
>
I'm really sorry for the mess... I accidentally sent two submissions with a
wrong From: address. Please disregard them.

-- 
Jan Synacek
Software Engineer, Red Hat

On Wed, Jan 22, 2020 at 10:16:14AM +0100, Jan Synacek
wrote:
> From: Jan Synacek <jan.synacek@redhat.com>
> 
> ---
>  daemon/listfs.ml          | 19 ++++++++++++++++---
>  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, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/daemon/listfs.ml b/daemon/listfs.ml
> index bf4dca6d4..a618513e8 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
> @@ -30,6 +31,7 @@ let rec list_filesystems () >  
>    (* Devices. *)
>    let devices = Devsparts.list_devices () in
> +
Did you mean to add a blank line here?
>    let devices = List.filter is_not_partitioned_device devices in
>    let ret = List.filter_map check_with_vfs_type devices in
>  
> @@ -144,9 +146,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]
Now Some doesn't line up with let :-/
>    { 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 = "\
This is fine.

The rest of this patch is fine.

I'm losing track of what order these patches would be applied
in order to preserve git bisection.  Maybe submit the whole series
in one go for version 2?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Jan 22, 2020 at 10:54 AM Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Wed, Jan 22, 2020 at 10:16:14AM +0100, Jan Synacek wrote:
> > From: Jan Synacek <jan.synacek@redhat.com>
> >
> > ---
> >  daemon/listfs.ml          | 19 ++++++++++++++++---
> >  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, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/daemon/listfs.ml b/daemon/listfs.ml
> > index bf4dca6d4..a618513e8 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
> > @@ -30,6 +31,7 @@ let rec list_filesystems () > >
> >    (* Devices. *)
> >    let devices = Devsparts.list_devices () in
> > +
>
> Did you mean to add a blank line here?
>
No, I didn't notice. I'll fix it.
>
> >    let devices = List.filter is_not_partitioned_device devices in
> >    let ret = List.filter_map check_with_vfs_type devices in
> >
> > @@ -144,9 +146,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]
>
> Now Some doesn't line up with let :-/
>
Will fix.

> >    { 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 = "\
>
> This is fine.
>
> The rest of this patch is fine.
>
> I'm losing track of what order these patches would be applied
> in order to preserve git bisection.  Maybe submit the whole series
> in one go for version 2?
>
The first to apply should be the one that patches the common/ subrepo. I'll
submit them all at once next time.

And again, I'm sorry for the mess with the incorrect submissions. I
didn't
notice that my patches had a wrong address in them.

Thank you for the review!
-- 
Jan Synacek
Software Engineer, Red Hat