Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/crypto.c | 85 ++++++++++++++++++++++++++++++++++----------
qapi/block-core.json | 7 ++--
2 files changed, 71 insertions(+), 21 deletions(-)
diff --git a/block/crypto.c b/block/crypto.c
index f42fa057e6..5905f7f520 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -534,6 +534,17 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
assert(create_options->driver == BLOCKDEV_DRIVER_LUKS);
luks_opts = &create_options->u.luks;
+ if (!luks_opts->has_size) {
+ error_setg(errp, "'size' is manadatory for image creation");
+ return -EINVAL;
+ }
+
+ if (!luks_opts->has_file) {
+ error_setg(errp, "'file' is manadatory for image creation");
+ return -EINVAL;
+ }
+
+
bs = bdrv_open_blockdev_ref(luks_opts->file, errp);
if (bs == NULL) {
return -EIO;
@@ -667,6 +678,39 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
}
+static int
+block_crypto_amend_options_generic(BlockDriverState *bs,
+ QCryptoBlockCreateOptions *amend_options,
+ bool force,
+ Error **errp)
+{
+ BlockCrypto *crypto = bs->opaque;
+ int ret = -1;
+
+ assert(crypto);
+ assert(crypto->block);
+
+ /* apply for exclusive write permissions to the underlying file*/
+ crypto->updating_keys = true;
+ ret = bdrv_child_refresh_perms(bs, bs->file, errp);
+ if (ret) {
+ goto cleanup;
+ }
+
+ ret = qcrypto_block_amend_options(crypto->block,
+ block_crypto_read_func,
+ block_crypto_write_func,
+ bs,
+ amend_options,
+ force,
+ errp);
+cleanup:
+ /* release exclusive write permissions to the underlying file*/
+ crypto->updating_keys = false;
+ bdrv_child_refresh_perms(bs, bs->file, errp);
+ return ret;
+}
+
static int
block_crypto_amend_options(BlockDriverState *bs,
QemuOpts *opts,
@@ -678,44 +722,45 @@ block_crypto_amend_options(BlockDriverState *bs,
BlockCrypto *crypto = bs->opaque;
QDict *cryptoopts = NULL;
QCryptoBlockCreateOptions *amend_options = NULL;
- int ret;
+ int ret = -EINVAL;
assert(crypto);
assert(crypto->block);
- crypto->updating_keys = true;
-
- ret = bdrv_child_refresh_perms(bs, bs->file, errp);
- if (ret < 0) {
- goto cleanup;
- }
-
cryptoopts = qemu_opts_to_qdict_filtered(opts, NULL,
&block_crypto_create_opts_luks,
true);
qdict_put_str(cryptoopts, "format", "luks");
amend_options = block_crypto_create_opts_init(cryptoopts, errp);
+
if (!amend_options) {
- ret = -EINVAL;
goto cleanup;
}
- ret = qcrypto_block_amend_options(crypto->block,
- block_crypto_read_func,
- block_crypto_write_func,
- bs,
- amend_options,
- force,
- errp);
+ ret = block_crypto_amend_options_generic(bs, amend_options, force, errp);
cleanup:
- crypto->updating_keys = false;
- bdrv_child_refresh_perms(bs, bs->file, errp);
qapi_free_QCryptoBlockCreateOptions(amend_options);
qobject_unref(cryptoopts);
return ret;
}
+static int
+coroutine_fn block_crypto_co_amend(BlockDriverState *bs,
+ BlockdevCreateOptions *opts,
+ bool force,
+ Error **errp)
+{
+ QCryptoBlockCreateOptions amend_opts;
+
+ amend_opts = (QCryptoBlockCreateOptions) {
+ .format = Q_CRYPTO_BLOCK_FORMAT_LUKS,
+ .u.luks = *qapi_BlockdevCreateOptionsLUKS_base(&opts->u.luks),
+ };
+
+ return block_crypto_amend_options_generic(bs, &amend_opts, force, errp);
+}
+
static void
block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
@@ -750,7 +795,8 @@ block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
*/
if (crypto->updating_keys) {
- /*need exclusive write access for header update */
+ assert(!(bs->open_flags & BDRV_O_NO_IO));
+ /*need exclusive read and write access for header update */
perm |= BLK_PERM_WRITE;
shared &= ~(BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE);
}
@@ -786,6 +832,7 @@ static BlockDriver bdrv_crypto_luks = {
.bdrv_get_info = block_crypto_get_info_luks,
.bdrv_get_specific_info = block_crypto_get_specific_info_luks,
.bdrv_amend_options = block_crypto_amend_options,
+ .bdrv_co_amend = block_crypto_co_amend,
.strong_runtime_opts = block_crypto_strong_runtime_opts,
};
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7900914506..4a6db98938 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4211,8 +4211,11 @@
# Driver specific image creation options for LUKS.
#
# @file Node to create the image format on
+# Mandatory for create
# @size Size of the virtual disk in bytes
+# Mandatory for create
# @preallocation Preallocation mode for the new image
+# Only for create
# (since: 4.2)
# (default: off; allowed values: off, metadata, falloc, full)
#
@@ -4220,8 +4223,8 @@
##
{ 'struct': 'BlockdevCreateOptionsLUKS',
'base': 'QCryptoBlockCreateOptionsLUKS',
- 'data': { 'file': 'BlockdevRef',
- 'size': 'size',
+ 'data': { '*file': 'BlockdevRef',
+ '*size': 'size',
'*preallocation': 'PreallocMode' } }
##
--
2.17.2
Maxim Levitsky <mlevitsk@redhat.com> writes:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7900914506..4a6db98938 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4211,8 +4211,11 @@
> # Driver specific image creation options for LUKS.
> #
> # @file Node to create the image format on
> +# Mandatory for create
> # @size Size of the virtual disk in bytes
> +# Mandatory for create
> # @preallocation Preallocation mode for the new image
> +# Only for create
> # (since: 4.2)
> # (default: off; allowed values: off, metadata, falloc, full)
> #
> @@ -4220,8 +4223,8 @@
> ##
> { 'struct': 'BlockdevCreateOptionsLUKS',
> 'base': 'QCryptoBlockCreateOptionsLUKS',
> - 'data': { 'file': 'BlockdevRef',
> - 'size': 'size',
> + 'data': { '*file': 'BlockdevRef',
> + '*size': 'size',
> '*preallocation': 'PreallocMode' } }
>
> ##
Why is this change needed?
When the commit message says "implement FOO" and nothing else, then I
don't expect QAPI schema changes. Working the answer to my question
into the commit message might avoid the surprise.
On Mon, 2019-10-07 at 09:58 +0200, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
>
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
>
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 7900914506..4a6db98938 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4211,8 +4211,11 @@
> > # Driver specific image creation options for LUKS.
> > #
> > # @file Node to create the image format on
> > +# Mandatory for create
> > # @size Size of the virtual disk in bytes
> > +# Mandatory for create
> > # @preallocation Preallocation mode for the new image
> > +# Only for create
> > # (since: 4.2)
> > # (default: off; allowed values: off, metadata, falloc, full)
> > #
> > @@ -4220,8 +4223,8 @@
> > ##
> > { 'struct': 'BlockdevCreateOptionsLUKS',
> > 'base': 'QCryptoBlockCreateOptionsLUKS',
> > - 'data': { 'file': 'BlockdevRef',
> > - 'size': 'size',
> > + 'data': { '*file': 'BlockdevRef',
> > + '*size': 'size',
> > '*preallocation': 'PreallocMode' } }
> >
> > ##
>
> Why is this change needed?
>
> When the commit message says "implement FOO" and nothing else, then I
> don't expect QAPI schema changes. Working the answer to my question
> into the commit message might avoid the surprise.
It is because for the amend interface, it doesn't make much sense to pass the size,
and the underlying block device.
Thats why I made these optional but I check that these parameters are present on creation
and not present on amend.
Still I am more and more inclined to think that I should not use the create options for amend,
but rather split into a new structure. This is just not worth it IMHO.
Best regards,
Maxim Levitsky
© 2016 - 2025 Red Hat, Inc.