From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aT7Vb-0007C7-HA for qemu-devel@nongnu.org; Tue, 09 Feb 2016 07:35:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aT7VX-0001RU-Pa for qemu-devel@nongnu.org; Tue, 09 Feb 2016 07:35:07 -0500 Date: Tue, 9 Feb 2016 12:34:56 +0000 From: "Daniel P. Berrange" Message-ID: <20160209123456.GH24614@redhat.com> References: <1453311539-1193-1-git-send-email-berrange@redhat.com> <1453311539-1193-17-git-send-email-berrange@redhat.com> <56B9075C.5060406@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <56B9075C.5060406@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 16/17] block: remove all encryption handling APIs Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Kevin Wolf , Fam Zheng , qemu-devel@nongnu.org, qemu-block@nongnu.org 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 > > --- > > > @@ -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 :|