Re: [Qemu-devel] [PATCH v1 15/15] block: remove all encryption handling APIs

2017-01-24 Thread Daniel P. Berrange
On Sat, Jan 21, 2017 at 08:22:53PM +0100, Max Reitz wrote:
> On 03.01.2017 19:28, Daniel P. Berrange wrote:
> > Now that all encryption keys must be provided upfront via
> > the QCryptoSecret API and associated block driver properties
> > there is no need for any explicit encryption handling APIs
> > in the block layer. Encryption can be handled transparently
> > within the block driver. We only retain an API for querying
> > whether an image is encrypted or not, since that is a
> > potentially useful piece of metadata to report to the user.
> > 
> > Signed-off-by: Daniel P. Berrange 
> > ---
> >  block.c   | 77 
> > +--
> >  block/crypto.c|  1 -
> >  block/qapi.c  |  2 +-
> >  block/qcow.c  |  1 -
> >  block/qcow2.c |  1 -
> >  blockdev.c| 37 ++-
> >  include/block/block.h |  3 --
> >  include/block/block_int.h |  1 -
> >  include/qapi/error.h  |  1 -
> >  qapi/common.json  |  5 +--
> >  10 files changed, 5 insertions(+), 124 deletions(-)
> 
> It would probably make sense to replace the description of
> BlockDeviceInfo's @encryption_key_missing in qapi/block-core.json by
> "Deprecated; always false".

Oh yes, that makes sense.


> > +error_setg_errno(errp, -ENOSYS,
> > + "Setting block passwords directly is no longer 
> > supported");
> 
> A plain error_setg() without _errno should be sufficient.

Will change it.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|



Re: [Qemu-devel] [PATCH v1 15/15] block: remove all encryption handling APIs

2017-01-21 Thread Max Reitz
On 03.01.2017 19:28, Daniel P. Berrange wrote:
> Now that all encryption keys must be provided upfront via
> the QCryptoSecret API and associated block driver properties
> there is no need for any explicit encryption handling APIs
> in the block layer. Encryption can be handled transparently
> within the block driver. We only retain an API for querying
> whether an image is encrypted or not, since that is a
> potentially useful piece of metadata to report to the user.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block.c   | 77 
> +--
>  block/crypto.c|  1 -
>  block/qapi.c  |  2 +-
>  block/qcow.c  |  1 -
>  block/qcow2.c |  1 -
>  blockdev.c| 37 ++-
>  include/block/block.h |  3 --
>  include/block/block_int.h |  1 -
>  include/qapi/error.h  |  1 -
>  qapi/common.json  |  5 +--
>  10 files changed, 5 insertions(+), 124 deletions(-)

It would probably make sense to replace the description of
BlockDeviceInfo's @encryption_key_missing in qapi/block-core.json by
"Deprecated; always false".

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 245e1e1..dfeba0c 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> @@ -2244,24 +2240,8 @@ void qmp_block_passwd(bool has_device, const char 
> *device,
>bool has_node_name, const char *node_name,
>const char *password, Error **errp)
>  {
> -Error *local_err = NULL;
> -BlockDriverState *bs;
> -AioContext *aio_context;
> -
> -bs = bdrv_lookup_bs(has_device ? device : NULL,
> -has_node_name ? node_name : NULL,
> -&local_err);
> -if (local_err) {
> -error_propagate(errp, local_err);
> -return;
> -}
> -
> -aio_context = bdrv_get_aio_context(bs);
> -aio_context_acquire(aio_context);
> -
> -bdrv_add_key(bs, password, errp);
> -
> -aio_context_release(aio_context);
> +error_setg_errno(errp, -ENOSYS,
> + "Setting block passwords directly is no longer 
> supported");

A plain error_setg() without _errno should be sufficient.

>  }
>  

I'll leave it up to you whether you want to follow the suggestions I've
given, so:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature


[Qemu-devel] [PATCH v1 15/15] block: remove all encryption handling APIs

2017-01-03 Thread Daniel P. Berrange
Now that all encryption keys must be provided upfront via
the QCryptoSecret API and associated block driver properties
there is no need for any explicit encryption handling APIs
in the block layer. Encryption can be handled transparently
within the block driver. We only retain an API for querying
whether an image is encrypted or not, since that is a
potentially useful piece of metadata to report to the user.

Signed-off-by: Daniel P. Berrange 
---
 block.c   | 77 +--
 block/crypto.c|  1 -
 block/qapi.c  |  2 +-
 block/qcow.c  |  1 -
 block/qcow2.c |  1 -
 blockdev.c| 37 ++-
 include/block/block.h |  3 --
 include/block/block_int.h |  1 -
 include/qapi/error.h  |  1 -
 qapi/common.json  |  5 +--
 10 files changed, 5 insertions(+), 124 deletions(-)

diff --git a/block.c b/block.c
index 39ddea3..27cca49 100644
--- a/block.c
+++ b/block.c
@@ -1865,15 +1865,7 @@ static BlockDriverState *bdrv_open_inherit(const char 
*filename,
 goto close_and_fail;
 }
 
-if (!bdrv_key_required(bs)) {
-bdrv_parent_cb_change_media(bs, true);
-} else if (!runstate_check(RUN_STATE_PRELAUNCH)
-   && !runstate_check(RUN_STATE_INMIGRATE)
-   && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */
-error_setg(errp,
-   "Guest must be stopped for opening of encrypted image");
-goto close_and_fail;
-}
+bdrv_parent_cb_change_media(bs, true);
 
 QDECREF(options);
 
@@ -2354,7 +2346,6 @@ static void bdrv_close(BlockDriverState *bs)
 bs->backing_format[0] = '\0';
 bs->total_sectors = 0;
 bs->encrypted = false;
-bs->valid_key = false;
 bs->sg = false;
 QDECREF(bs->options);
 QDECREF(bs->explicit_options);
@@ -2723,72 +2714,6 @@ bool bdrv_is_encrypted(BlockDriverState *bs)
 return bs->encrypted;
 }
 
-bool bdrv_key_required(BlockDriverState *bs)
-{
-BdrvChild *backing = bs->backing;
-
-if (backing && backing->bs->encrypted && !backing->bs->valid_key) {
-return true;
-}
-return (bs->encrypted && !bs->valid_key);
-}
-
-int bdrv_set_key(BlockDriverState *bs, const char *key)
-{
-int ret;
-if (bs->backing && bs->backing->bs->encrypted) {
-ret = bdrv_set_key(bs->backing->bs, key);
-if (ret < 0)
-return ret;
-if (!bs->encrypted)
-return 0;
-}
-if (!bs->encrypted) {
-return -EINVAL;
-} else if (!bs->drv || !bs->drv->bdrv_set_key) {
-return -ENOMEDIUM;
-}
-ret = bs->drv->bdrv_set_key(bs, key);
-if (ret < 0) {
-bs->valid_key = false;
-} else if (!bs->valid_key) {
-/* call the change callback now, we skipped it on open */
-bs->valid_key = true;
-bdrv_parent_cb_change_media(bs, true);
-}
-return ret;
-}
-
-/*
- * Provide an encryption key for @bs.
- * If @key is non-null:
- * If @bs is not encrypted, fail.
- * Else if the key is invalid, fail.
- * Else set @bs's key to @key, replacing the existing key, if any.
- * If @key is null:
- * If @bs is encrypted and still lacks a key, fail.
- * Else do nothing.
- * On failure, store an error object through @errp if non-null.
- */
-void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
-{
-if (key) {
-if (!bdrv_is_encrypted(bs)) {
-error_setg(errp, "Node '%s' is not encrypted",
-  bdrv_get_device_or_node_name(bs));
-} else if (bdrv_set_key(bs, key) < 0) {
-error_setg(errp, QERR_INVALID_PASSWORD);
-}
-} else {
-if (bdrv_key_required(bs)) {
-error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
-  "'%s' (%s) is encrypted",
-  bdrv_get_device_or_node_name(bs),
-  bdrv_get_encrypted_filename(bs));
-}
-}
-}
-
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
 return bs->drv ? bs->drv->format_name : NULL;
diff --git a/block/crypto.c b/block/crypto.c
index 915ba70..2c4279c 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -380,7 +380,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 }
 
 bs->encrypted = true;
-bs->valid_key = true;
 
 ret = 0;
  cleanup:
diff --git a/block/qapi.c b/block/qapi.c
index a62e862..68cab56 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -45,7 +45,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
 info->ro = bs->read_only;
 info->drv= g_strdup(bs->drv->format_name);
 info->encrypted  = bs->encrypted;
-info->encryption_key_missing = bdrv_key_required(bs);
+info->encryption_key_missing = false;
 
 info->cache = g_new(BlockdevCacheInfo, 1);
 *info->cache = (BlockdevCacheInfo) {
diff --git a/bloc