Re: [Qemu-devel] [PATCH v2 16/17] block: remove all encryption handling APIs

["Daniel P. Berrange" <berrange@redhat.com>]

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