Re: [Qemu-devel] [PATCH 08/10] block/crypto: implement blockdev-amend

2019-09-12 Thread Maxim Levitsky
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 
> > ---
> >  block/crypto.c   | 86 +---
> >  qapi/block-core.json |  4 +--
> >  2 files changed, 68 insertions(+), 22 deletions(-)
> 
> Reviewed-by: Daniel P. Berrangé 
> 
> 
> >  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,
> >   
> > _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(>u.luks),
> > +};
> > +
> > +return block_crypto_amend_options_generic(bs, _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




Re: [Qemu-devel] [PATCH 08/10] block/crypto: implement blockdev-amend

2019-09-06 Thread Daniel P . Berrangé
On Fri, Aug 30, 2019 at 11:56:06PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky 
> ---
>  block/crypto.c   | 86 +---
>  qapi/block-core.json |  4 +--
>  2 files changed, 68 insertions(+), 22 deletions(-)

Reviewed-by: Daniel P. Berrangé 


>  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,
>   _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(>u.luks),
> +};
> +
> +return block_crypto_amend_options_generic(bs, _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
-- 
|: 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 :|