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 <berra...@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
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
