Re: [Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_generic()

[Kevin Wolf <kwolf@redhat.com>]

Am 14.05.2019 um 13:13 hat Daniel P. Berrangé geschrieben:
> On Mon, Mar 12, 2018 at 04:02:14PM +0100, Kevin Wolf wrote:
> > Everything that refers to the protocol layer or QemuOpts is moved out of
> > block_crypto_create_generic(), so that the remaining function is
> > suitable to be called by a .bdrv_co_create implementation.
> > 
> > LUKS is the only driver that actually implements the old interface, and
> > we don't intend to use it in any new drivers, so put the moved out code
> > directly into a LUKS function rather than creating a generic
> > intermediate one.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/crypto.c | 95 +++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 61 insertions(+), 34 deletions(-)
> 
> 
> Reviving a year old commit...
> 
> The LUKS driver doesn't implement preallocation during create.
> 
> Before this commit this would be reported
> 
>  $ qemu-img create -f luks --object secret,id=sec0,data=base -o key-secret=sec0 base.luks 1G -o preallocation=full
>  Formatting 'base.luks', fmt=luks size=1073741824 key-secret=sec0 preallocation=full
>  qemu-img: base.luks: Parameter 'preallocation' is unexpected
> 
> After this commit, there is no error reported - it just silently
> ignores the preallocation=full option.
> 
> I'm a bit lost in block layer understanding where is the right
> place to fix the error reporting in this case.

Hmm, this looked strange at first, but I see now where the difference
is.

In the old state, crypto used qemu_opts_to_qdict() and then fed all
options to its own visitor. I'm not sure whether I can clearly call this
a bug, but it's different from other format drivers, which parse all
options they know and pass the rest to the protocol driver.

The new version was converted to use qemu_opts_to_qdict_filtered() like
in all other drivers, so we got the same behaviour in crypto: Unknown
options are passed to the protocol.

As it happens, file-posix does support 'preallocation', so the operation
will return success now. Of course, passing the preallocation to the
protocol level is questionable for non-raw image formats, and completely
useless when you create the protocol level with size 0 and you'll call
blk_truncate() later.

I think we would get back to the old state if you just changed the
qemu_opts_to_qdict_filtered() call back to qemu_opts_to_qdict(). But it
make the driver inconsistent with other drivers again.

Maybe the best way to solve this would be to just implement
preallocation. It should be as easy as adding a 'preallocation' create
option (in QAPI and block_crypto_create_opts_luks), putting the
PreallocMode into BlockCryptoCreateData and then passing it to the
blk_truncate() call in block_crypto_init_func().

Kevin