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(-)
diff --git a/block/crypto.c b/block/crypto.c
index 77871640cc..b0a4cb3388 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -306,43 +306,29 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
}
-static int block_crypto_create_generic(QCryptoBlockFormat format,
- const char *filename,
- QemuOpts *opts,
- Error **errp)
+static int block_crypto_co_create_generic(BlockDriverState *bs,
+ int64_t size,
+ QCryptoBlockCreateOptions *opts,
+ Error **errp)
{
- int ret = -EINVAL;
- QCryptoBlockCreateOptions *create_opts = NULL;
+ int ret;
+ BlockBackend *blk;
QCryptoBlock *crypto = NULL;
- struct BlockCryptoCreateData data = {
- .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
- BDRV_SECTOR_SIZE),
- };
- QDict *cryptoopts;
-
- /* Parse options */
- cryptoopts = qemu_opts_to_qdict(opts, NULL);
+ struct BlockCryptoCreateData data;
- create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
- if (!create_opts) {
- return -1;
- }
+ blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
- /* Create protocol layer */
- ret = bdrv_create_file(filename, opts, errp);
+ ret = blk_insert_bs(blk, bs, errp);
if (ret < 0) {
- return ret;
+ goto cleanup;
}
- data.blk = blk_new_open(filename, NULL, NULL,
- BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
- errp);
- if (!data.blk) {
- return -EINVAL;
- }
+ data = (struct BlockCryptoCreateData) {
+ .blk = blk,
+ .size = size,
+ };
- /* Create format layer */
- crypto = qcrypto_block_create(create_opts, NULL,
+ crypto = qcrypto_block_create(opts, NULL,
block_crypto_init_func,
block_crypto_write_func,
&data,
@@ -355,10 +341,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
ret = 0;
cleanup:
- QDECREF(cryptoopts);
qcrypto_block_free(crypto);
- blk_unref(data.blk);
- qapi_free_QCryptoBlockCreateOptions(create_opts);
+ blk_unref(blk);
return ret;
}
@@ -563,8 +547,51 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
QemuOpts *opts,
Error **errp)
{
- return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS,
- filename, opts, errp);
+ QCryptoBlockCreateOptions *create_opts = NULL;
+ BlockDriverState *bs = NULL;
+ QDict *cryptoopts;
+ int64_t size;
+ int ret;
+
+ /* Parse options */
+ size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
+
+ cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
+ &block_crypto_create_opts_luks,
+ true);
+
+ create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
+ cryptoopts, errp);
+ if (!create_opts) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create protocol layer */
+ ret = bdrv_create_file(filename, opts, errp);
+ if (ret < 0) {
+ return ret;
+ }
+
+ bs = bdrv_open(filename, NULL, NULL,
+ BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
+ if (!bs) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Create format layer */
+ ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ ret = 0;
+fail:
+ bdrv_unref(bs);
+ qapi_free_QCryptoBlockCreateOptions(create_opts);
+ QDECREF(cryptoopts);
+ return ret;
}
static int block_crypto_get_info_luks(BlockDriverState *bs,
--
2.13.6
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.
>
> diff --git a/block/crypto.c b/block/crypto.c
> index 77871640cc..b0a4cb3388 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -306,43 +306,29 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
> }
>
>
> -static int block_crypto_create_generic(QCryptoBlockFormat format,
> - const char *filename,
> - QemuOpts *opts,
> - Error **errp)
> +static int block_crypto_co_create_generic(BlockDriverState *bs,
> + int64_t size,
> + QCryptoBlockCreateOptions *opts,
> + Error **errp)
> {
> - int ret = -EINVAL;
> - QCryptoBlockCreateOptions *create_opts = NULL;
> + int ret;
> + BlockBackend *blk;
> QCryptoBlock *crypto = NULL;
> - struct BlockCryptoCreateData data = {
> - .size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
> - BDRV_SECTOR_SIZE),
> - };
> - QDict *cryptoopts;
> -
> - /* Parse options */
> - cryptoopts = qemu_opts_to_qdict(opts, NULL);
> + struct BlockCryptoCreateData data;
>
> - create_opts = block_crypto_create_opts_init(format, cryptoopts, errp);
> - if (!create_opts) {
> - return -1;
> - }
> + blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL);
>
> - /* Create protocol layer */
> - ret = bdrv_create_file(filename, opts, errp);
> + ret = blk_insert_bs(blk, bs, errp);
> if (ret < 0) {
> - return ret;
> + goto cleanup;
> }
>
> - data.blk = blk_new_open(filename, NULL, NULL,
> - BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL,
> - errp);
> - if (!data.blk) {
> - return -EINVAL;
> - }
> + data = (struct BlockCryptoCreateData) {
> + .blk = blk,
> + .size = size,
> + };
>
> - /* Create format layer */
> - crypto = qcrypto_block_create(create_opts, NULL,
> + crypto = qcrypto_block_create(opts, NULL,
> block_crypto_init_func,
> block_crypto_write_func,
> &data,
> @@ -355,10 +341,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
>
> ret = 0;
> cleanup:
> - QDECREF(cryptoopts);
> qcrypto_block_free(crypto);
> - blk_unref(data.blk);
> - qapi_free_QCryptoBlockCreateOptions(create_opts);
> + blk_unref(blk);
> return ret;
> }
>
> @@ -563,8 +547,51 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
> QemuOpts *opts,
> Error **errp)
> {
> - return block_crypto_create_generic(Q_CRYPTO_BLOCK_FORMAT_LUKS,
> - filename, opts, errp);
> + QCryptoBlockCreateOptions *create_opts = NULL;
> + BlockDriverState *bs = NULL;
> + QDict *cryptoopts;
> + int64_t size;
> + int ret;
> +
> + /* Parse options */
> + size = qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0);
> +
> + cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
> + &block_crypto_create_opts_luks,
> + true);
> +
> + create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
> + cryptoopts, errp);
> + if (!create_opts) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Create protocol layer */
> + ret = bdrv_create_file(filename, opts, errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + bs = bdrv_open(filename, NULL, NULL,
> + BDRV_O_RDWR | BDRV_O_RESIZE | BDRV_O_PROTOCOL, errp);
> + if (!bs) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Create format layer */
> + ret = block_crypto_co_create_generic(bs, size, create_opts, errp);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> + ret = 0;
> +fail:
> + bdrv_unref(bs);
> + qapi_free_QCryptoBlockCreateOptions(create_opts);
> + QDECREF(cryptoopts);
> + return ret;
> }
>
> static int block_crypto_get_info_luks(BlockDriverState *bs,
> --
> 2.13.6
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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
© 2016 - 2026 Red Hat, Inc.