diff mbox series

[v2,08/11] block/crypto: implement blockdev-amend

Message ID 20190912223028.18496-9-mlevitsk@redhat.com (mailing list archive)
State New, archived
Headers show
Series RFC crypto/luks: encryption key managment using amend interface | expand

Commit Message

Maxim Levitsky Sept. 12, 2019, 10:30 p.m. UTC
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(-)

Comments

Markus Armbruster Oct. 7, 2019, 7:58 a.m. UTC | #1
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.
Maxim Levitsky Nov. 8, 2019, 3:36 p.m. UTC | #2
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
diff mbox series

Patch

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' } }
 
 ##