| Message ID | 1453311539-1193-17-git-send-email-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2016 10:38 AM, 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 <berrange@redhat.com> > --- > @@ -2190,7 +2181,6 @@ void bdrv_close(BlockDriverState *bs) > bs->backing_format[0] = '\0'; > bs->total_sectors = 0; > bs->encrypted = 0; > - bs->valid_key = 0; It would be nice if this patch (or maybe a followup) switched bs->encrypted to bool, rather than int. > -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); If I'm not mistaken, this was the only use of QERR_INVALID_PASSWORD; please also nuke it from include/qapi/qmp/qerror.h. > - } > - } else { > - if (bdrv_key_required(bs)) { > - error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED, > - "'%s' (%s) is encrypted", Likewise, this looks like the last use of ERROR_CLASS_DEVICE_ENCRYPTED (since 15/17 nuked the only client in hmp.c that cared about the error class); it would be nice to nuke the remains in include/qapi/error.h and in qapi/common.json. > +++ b/blockdev.c > @@ -2261,24 +2257,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"); We should document in qapi/block-core.json that the QMP 'block_passwd' command is deprecated, and will be removed in a future release (but I suspect we don't want to completely remove it in 2.6, so that we at least have time to find clients that were relying on it and should be using the new methods). > +++ b/include/block/block_int.h > @@ -374,7 +374,6 @@ struct BlockDriverState { > int read_only; /* if true, the media is read only */ > int open_flags; /* flags used to open the file, re-used for re-open */ > int encrypted; /* if true, the media is encrypted */ > - int valid_key; /* if true, a valid encryption key has been set */ > int sg; /* if true, the device is a /dev/sg* */ > int copy_on_read; /* if true, copy read backing sectors into image > note this is a reference count */ Hmm - several variables that are only 'true' or 'false' and should be typed 'bool'. Only 'encrypted' is a candidate for change in this patch; 'sg' is unrelated. And 'copy_on_read' should be tweaked to document 'nonzero', not 'true', since it is used as a reference count rather than bool.
On Mon, Feb 08, 2016 at 02:23:40PM -0700, Eric Blake wrote: > On 01/20/2016 10:38 AM, 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 <berrange@redhat.com> > > --- > > > @@ -2190,7 +2181,6 @@ void bdrv_close(BlockDriverState *bs) > > bs->backing_format[0] = '\0'; > > bs->total_sectors = 0; > > bs->encrypted = 0; > > - bs->valid_key = 0; > > It would be nice if this patch (or maybe a followup) switched > bs->encrypted to bool, rather than int. I'll prefer todo it later to avoid mixing unrelated changes. > > -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); > > If I'm not mistaken, this was the only use of QERR_INVALID_PASSWORD; > please also nuke it from include/qapi/qmp/qerror.h. > > > - } > > - } else { > > - if (bdrv_key_required(bs)) { > > - error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED, > > - "'%s' (%s) is encrypted", > > Likewise, this looks like the last use of ERROR_CLASS_DEVICE_ENCRYPTED > (since 15/17 nuked the only client in hmp.c that cared about the error > class); it would be nice to nuke the remains in include/qapi/error.h and > in qapi/common.json. Oh yes, goo points. > > +++ b/blockdev.c > > @@ -2261,24 +2257,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"); > > We should document in qapi/block-core.json that the QMP 'block_passwd' > command is deprecated, and will be removed in a future release (but I > suspect we don't want to completely remove it in 2.6, so that we at > least have time to find clients that were relying on it and should be > using the new methods). I thought that I had mentioned that already, but will do so if its not already there. > > +++ b/include/block/block_int.h > > @@ -374,7 +374,6 @@ struct BlockDriverState { > > int read_only; /* if true, the media is read only */ > > int open_flags; /* flags used to open the file, re-used for re-open */ > > int encrypted; /* if true, the media is encrypted */ > > - int valid_key; /* if true, a valid encryption key has been set */ > > int sg; /* if true, the device is a /dev/sg* */ > > int copy_on_read; /* if true, copy read backing sectors into image > > note this is a reference count */ > > Hmm - several variables that are only 'true' or 'false' and should be > typed 'bool'. Only 'encrypted' is a candidate for change in this patch; > 'sg' is unrelated. And 'copy_on_read' should be tweaked to document > 'nonzero', not 'true', since it is used as a reference count rather than > bool. Yeah, these should all be changed separately really. Regards, Daniel
diff --git a/block.c b/block.c index 0806d4f..5403355 100644 --- a/block.c +++ b/block.c @@ -1693,17 +1693,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, goto close_and_fail; } - if (!bdrv_key_required(bs)) { - if (bs->blk) { - blk_dev_change_media_cb(bs->blk, 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"); - ret = -EBUSY; - goto close_and_fail; + if (bs->blk) { + blk_dev_change_media_cb(bs->blk, true); } QDECREF(options); @@ -2190,7 +2181,6 @@ void bdrv_close(BlockDriverState *bs) bs->backing_format[0] = '\0'; bs->total_sectors = 0; bs->encrypted = 0; - bs->valid_key = 0; bs->sg = 0; bs->zero_beyond_eof = false; QDECREF(bs->options); @@ -2774,74 +2764,6 @@ int bdrv_is_encrypted(BlockDriverState *bs) return bs->encrypted; } -int bdrv_key_required(BlockDriverState *bs) -{ - BdrvChild *backing = bs->backing; - - if (backing && backing->bs->encrypted && !backing->bs->valid_key) { - return 1; - } - 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 = 0; - } else if (!bs->valid_key) { - bs->valid_key = 1; - if (bs->blk) { - /* call the change callback now, we skipped it on open */ - blk_dev_change_media_cb(bs->blk, 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 2ba78bd..03194e5 100644 --- a/block/crypto.c +++ b/block/crypto.c @@ -278,7 +278,6 @@ static int block_crypto_open_generic(QCryptoBlockFormat format, } bs->encrypted = 1; - bs->valid_key = 1; qemu_co_mutex_init(&crypto->lock); diff --git a/block/qapi.c b/block/qapi.c index 58d3975..639b231 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -42,7 +42,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp) 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/block/qcow.c b/block/qcow.c index 3b7c2b9..2fc7c3c 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -212,7 +212,6 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } bs->encrypted = 1; - bs->valid_key = 1; } s->cluster_bits = header.cluster_bits; s->cluster_size = 1 << s->cluster_bits; diff --git a/block/qcow2.c b/block/qcow2.c index 5249ca2..1fbae85 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1210,7 +1210,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, s->crypt_method_header = header.crypt_method; if (s->crypt_method_header) { bs->encrypted = 1; - bs->valid_key = 1; } s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */ diff --git a/blockdev.c b/blockdev.c index 1392fff..d3c3317 100644 --- a/blockdev.c +++ b/blockdev.c @@ -623,10 +623,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts, bdrv_set_io_limits(bs, &cfg); } - if (bdrv_key_required(bs)) { - autostart = 0; - } - block_acct_init(blk_get_stats(blk), account_invalid, account_failed); if (!parse_stats_intervals(blk_get_stats(blk), interval_list, errp)) { @@ -2261,24 +2257,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"); } void qmp_blockdev_open_tray(const char *device, bool has_force, bool force, @@ -2505,12 +2485,6 @@ void qmp_blockdev_change_medium(const char *device, const char *filename, blk_apply_root_state(blk, medium_bs); - bdrv_add_key(medium_bs, NULL, &err); - if (err) { - error_propagate(errp, err); - goto fail; - } - qmp_blockdev_open_tray(device, false, false, &err); if (err) { error_propagate(errp, err); @@ -3849,16 +3823,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp) } } - if (bs && bdrv_key_required(bs)) { - if (blk) { - blk_unref(blk); - } else { - bdrv_unref(bs); - } - error_setg(errp, "blockdev-add doesn't support encrypted devices"); - goto fail; - } - fail: qmp_output_visitor_cleanup(ov); } diff --git a/include/block/block.h b/include/block/block.h index 73ffbd5..379a24c 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -414,10 +414,6 @@ bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base); BlockDriverState *bdrv_next_node(BlockDriverState *bs); BlockDriverState *bdrv_next(BlockDriverState *bs); int bdrv_is_encrypted(BlockDriverState *bs); -int bdrv_key_required(BlockDriverState *bs); -int bdrv_set_key(BlockDriverState *bs, const char *key); -void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp); -int bdrv_query_missing_keys(void); void bdrv_iterate_format(void (*it)(void *opaque, const char *name), void *opaque); const char *bdrv_get_node_name(const BlockDriverState *bs); diff --git a/include/block/block_int.h b/include/block/block_int.h index 256609d..191ce83 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -374,7 +374,6 @@ struct BlockDriverState { int read_only; /* if true, the media is read only */ int open_flags; /* flags used to open the file, re-used for re-open */ int encrypted; /* if true, the media is encrypted */ - int valid_key; /* if true, a valid encryption key has been set */ int sg; /* if true, the device is a /dev/sg* */ int copy_on_read; /* if true, copy read backing sectors into image note this is a reference count */
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 <berrange@redhat.com> --- block.c | 82 ++--------------------------------------------- block/crypto.c | 1 - block/qapi.c | 2 +- block/qcow.c | 1 - block/qcow2.c | 1 - blockdev.c | 40 ++--------------------- include/block/block.h | 4 --- include/block/block_int.h | 1 - 8 files changed, 5 insertions(+), 127 deletions(-)