[v2] LUKS: support preallocation in qemu-img

preallocation=off and preallocation=metadata
both allocate luks header only, and preallocation=falloc/full
is passed to underlying file.

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

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

On 11.07.19 11:11, Maxim Levitsky wrote:
> preallocation=off and preallocation=metadata
> both allocate luks header only, and preallocation=falloc/full
> is passed to underlying file.
> 
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)

FWIW, do you see the implementation of block_crypto_co_truncate()?
Like, how it just passes preallocation requests through to the
underlying layer?  How I said it shouldn’t be done?

Yes, that was me, in commit 7ea37c30660.

So, er, yeah.

> diff --git a/block/crypto.c b/block/crypto.c
> index 8237424ae6..cbc291301e 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
[...]

> @@ -534,12 +537,28 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
>      QCryptoBlockCreateOptions *create_opts = NULL;
>      BlockDriverState *bs = NULL;
>      QDict *cryptoopts;
> +    PreallocMode prealloc;
> +    char *buf = NULL;
>      int64_t size;
>      int ret;
> +    Error *local_err = NULL;
>  
>      /* Parse options */
>      size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
>  
> +    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> +    prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
> +                                   PREALLOC_MODE_OFF, &local_err);

Please align such lines to the opening parenthesis.

> +    g_free(buf);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
> +    }
> +
> +    if (prealloc == PREALLOC_MODE_METADATA) {
> +        prealloc  = PREALLOC_MODE_OFF;

There is one space too many here.

> +    }
> +

I think you also need to add a @preallocation parameter to
BlockdevCreateOptionsLUKS and handle it in block_crypto_co_create_luks().

Max

>      cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
>                                               &block_crypto_create_opts_luks,
>                                               true);

On Thu, 2019-07-11 at 15:43 +0200, Max Reitz wrote:
> On 11.07.19 11:11, Maxim Levitsky wrote:
> > preallocation=off and preallocation=metadata
> > both allocate luks header only, and preallocation=falloc/full
> > is passed to underlying file.
> > 
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1534951
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c | 25 ++++++++++++++++++++++---
> >  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> FWIW, do you see the implementation of block_crypto_co_truncate()?
> Like, how it just passes preallocation requests through to the
> underlying layer?  How I said it shouldn’t be done?
> 
> Yes, that was me, in commit 7ea37c30660.
> 
> So, er, yeah.
> 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 8237424ae6..cbc291301e 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> 
> [...]
> 
> > @@ -534,12 +537,28 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
> >      QCryptoBlockCreateOptions *create_opts = NULL;
> >      BlockDriverState *bs = NULL;
> >      QDict *cryptoopts;
> > +    PreallocMode prealloc;
> > +    char *buf = NULL;
> >      int64_t size;
> >      int ret;
> > +    Error *local_err = NULL;
> >  
> >      /* Parse options */
> >      size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> >  
> > +    buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
> > +    prealloc = qapi_enum_parse(&PreallocMode_lookup, buf,
> > +                                   PREALLOC_MODE_OFF, &local_err);
> 
> Please align such lines to the opening parenthesis.
True - I really need to invest some time to update the checkpatch.pl
in qemu source tree to be up to date, or find a way to use the kernel one,
it is so useful to let it catch these things instead of wasting your time.

> 
> > +    g_free(buf);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return -EINVAL;
> > +    }
> > +
> > +    if (prealloc == PREALLOC_MODE_METADATA) {
> > +        prealloc  = PREALLOC_MODE_OFF;
> 
> There is one space too many here.
Oops, same thing as above.

> 
> > +    }
> > +
> 
> I think you also need to add a @preallocation parameter to
> BlockdevCreateOptionsLUKS and handle it in block_crypto_co_create_luks().

I was under impression that with new qmp based blockdev-create api, the user
should pretty much do the preallocation itself on the underlying block file,
and then create the luks on it.

However I do see that qcow2 has a preallocation mode there, so I guess I was wrong.
Will do it now.

Best regards,
	Maxim Levitsky