Libguestfs - Jan 2020 - Re: [PATCH] mltools, options: support --allow-discards when decrypting LUKS devices

On Wed, Jan 22, 2020 at 10:14:38AM +0100, Jan Synacek
wrote:
> -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. *)
Documentation here needs a short explanation of what the
new allow_discards parameter does, and what the default is.
> diff --git a/options/decrypt.c b/options/decrypt.c
> index 683cf5e..0f24a7a 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, int 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);
Obviously this means this patch depends on the API change :-)

[...]
>  /* 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, int
allowdiscards);
> -  inspect_do_decrypt (g, ks);
> +  inspect_do_decrypt (g, ks, 0);
Kind of wonder if we want to use a C bool here instead of an int.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Jan 22, 2020 at 10:50 AM Richard W.M. Jones <rjones@redhat.com>
wrote:
> On Wed, Jan 22, 2020 at 10:14:38AM +0100, Jan Synacek wrote:
> > -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. *)
>
> Documentation here needs a short explanation of what the
> new allow_discards parameter does, and what the default is.
>
Will fix.
>
> > diff --git a/options/decrypt.c b/options/decrypt.c
> > index 683cf5e..0f24a7a 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, int
> 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);
>
> Obviously this means this patch depends on the API change :-)
>
Yes. Am I supposed to note that somewhere? Or did I miss anything?

[...]
> >  /* 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,
int
> allowdiscards);
>
> > -  inspect_do_decrypt (g, ks);
> > +  inspect_do_decrypt (g, ks, 0);
>
> Kind of wonder if we want to use a C bool here instead of an int.
>
I didn't notice that I could do that. But I think I've seen elsewhere in
the code that for "flags" like this, ints are used. The OCaml bools
also
translate to C ints 1:1, but I guess that's the case for stdbool booleans
too.

>
> Rich.
>
> --
> Richard Jones, Virtualization Group, Red Hat
> http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html
>
>
-- 
Jan Synacek
Software Engineer, Red Hat

On Wed, Jan 22, 2020 at 11:57:34AM +0100, Jan Synacek
wrote:
> > >  /* 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, int
> > allowdiscards);
> >
> > > -  inspect_do_decrypt (g, ks);
> > > +  inspect_do_decrypt (g, ks, 0);
> >
> > Kind of wonder if we want to use a C bool here instead of an int.
> >
> 
> I didn't notice that I could do that. But I think I've seen
elsewhere in
> the code that for "flags" like this, ints are used. The OCaml
bools also
> translate to C ints 1:1, but I guess that's the case for stdbool
booleans
> too.
The generator translates Bool/OBool to int for external
APIs (because it has to for backwards compatibility with
very old versions that predate our use of stdbool.h).  However
it could probably use bool internally (eg. for the daemon)
but doesn't at the moment.

However inspect_do_decrypt is an internal API between
parts of the tools, and also isn't generated, so we can
do whatever we want for this one.

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