[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_gene
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/6] luks: Create block_crypto_co_create_generic() |
Date: |
Tue, 14 May 2019 19:53:15 +0200 |
User-agent: |
Mutt/1.11.3 (2019-02-01) |
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 <address@hidden>
> > Reviewed-by: Daniel P. Berrangé <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> > 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