diff mbox series

[08/10] block/crypto: implement blockdev-amend

Message ID 20190830205608.18192-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 Aug. 30, 2019, 8:56 p.m. UTC 
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c       | 86 +++++++++++++++++++++++++++++++++-----------
 qapi/block-core.json |  4 +--
 2 files changed, 68 insertions(+), 22 deletions(-)

Comments

Daniel P. Berrangé Sept. 6, 2019, 2:10 p.m. UTC | #1 
On Fri, Aug 30, 2019 at 11:56:06PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c       | 86 +++++++++++++++++++++++++++++++++-----------
>  qapi/block-core.json |  4 +--
>  2 files changed, 68 insertions(+), 22 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


>  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;

nitpick - space before '='

>  
>      assert(crypto);
>      assert(crypto->block);
>  
> -    crypto->updating_keys = true;
> -
> -    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
> -    if (ret) {
> -        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;
> +        goto out;
>      }
>  
> -    ret = qcrypto_block_amend_options(crypto->block,
> -                                      block_crypto_read_func,
> -                                      block_crypto_write_func,
> -                                      bs,
> -                                      amend_options,
> -                                      force,
> -                                      errp);
> -cleanup:
> -    crypto->updating_keys = false;
> -    bdrv_child_refresh_perms(bs, bs->file, errp);
> +    ret = block_crypto_amend_options_generic(bs, amend_options, force, errp);
> +out:

No need to rename the "cleanup" label to "out"

>      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,
> @@ -774,6 +819,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..02375fb59a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4220,8 +4220,8 @@
>  ##
>  { 'struct': 'BlockdevCreateOptionsLUKS',
>    'base': 'QCryptoBlockCreateOptionsLUKS',
> -  'data': { 'file':             'BlockdevRef',
> -            'size':             'size',
> +  'data': { '*file':             'BlockdevRef',
> +            '*size':             'size',

Docs comment to explain they are mandatory for create 

>              '*preallocation':   'PreallocMode' } }
>  
>  ##
> -- 
> 2.17.2
> 

Regards,
Daniel
Maxim Levitsky Sept. 12, 2019, 7:18 p.m. UTC | #2 
On Fri, 2019-09-06 at 15:10 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 30, 2019 at 11:56:06PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/crypto.c       | 86 +++++++++++++++++++++++++++++++++-----------
> >  qapi/block-core.json |  4 +--
> >  2 files changed, 68 insertions(+), 22 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> >  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;
> 
> nitpick - space before '='
Done. This is one of the few errors that checkpatch.pl does catch,
but apparently I forgot to run it on this patch.
> 
> >  
> >      assert(crypto);
> >      assert(crypto->block);
> >  
> > -    crypto->updating_keys = true;
> > -
> > -    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
> > -    if (ret) {
> > -        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;
> > +        goto out;
> >      }
> >  
> > -    ret = qcrypto_block_amend_options(crypto->block,
> > -                                      block_crypto_read_func,
> > -                                      block_crypto_write_func,
> > -                                      bs,
> > -                                      amend_options,
> > -                                      force,
> > -                                      errp);
> > -cleanup:
> > -    crypto->updating_keys = false;
> > -    bdrv_child_refresh_perms(bs, bs->file, errp);
> > +    ret = block_crypto_amend_options_generic(bs, amend_options, force, errp);
> > +out:
> 
> No need to rename the "cleanup" label to "out"
All right.
> 
> >      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,
> > @@ -774,6 +819,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..02375fb59a 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4220,8 +4220,8 @@
> >  ##
> >  { 'struct': 'BlockdevCreateOptionsLUKS',
> >    'base': 'QCryptoBlockCreateOptionsLUKS',
> > -  'data': { 'file':             'BlockdevRef',
> > -            'size':             'size',
> > +  'data': { '*file':             'BlockdevRef',
> > +            '*size':             'size',
> 
> Docs comment to explain they are mandatory for create 
Done
> 
> >              '*preallocation':   'PreallocMode' } }
> >  
> >  ##
> > -- 
> > 2.17.2
> > 
> 
> Regards,
> Daniel

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/block/crypto.c b/block/crypto.c
index dbd95a99ba..9cb668ff0e 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) {
-        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;
+        goto out;
     }
 
-    ret = qcrypto_block_amend_options(crypto->block,
-                                      block_crypto_read_func,
-                                      block_crypto_write_func,
-                                      bs,
-                                      amend_options,
-                                      force,
-                                      errp);
-cleanup:
-    crypto->updating_keys = false;
-    bdrv_child_refresh_perms(bs, bs->file, errp);
+    ret = block_crypto_amend_options_generic(bs, amend_options, force, errp);
+out:
     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,
@@ -774,6 +819,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..02375fb59a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4220,8 +4220,8 @@ 
 ##
 { 'struct': 'BlockdevCreateOptionsLUKS',
   'base': 'QCryptoBlockCreateOptionsLUKS',
-  'data': { 'file':             'BlockdevRef',
-            'size':             'size',
+  'data': { '*file':             'BlockdevRef',
+            '*size':             'size',
             '*preallocation':   'PreallocMode' } }
 
 ##