From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50281) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwZiV-0005dU-9g for qemu-devel@nongnu.org; Wed, 01 Aug 2012 10:16:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwZiP-0000R3-JY for qemu-devel@nongnu.org; Wed, 01 Aug 2012 10:16:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:19369) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwZiP-0000Qz-Am for qemu-devel@nongnu.org; Wed, 01 Aug 2012 10:15:57 -0400 Date: Wed, 1 Aug 2012 10:47:45 -0300 From: Luiz Capitulino Message-ID: <20120801104745.455b18a5@doriath.home> In-Reply-To: <877gtihmgn.fsf@blackfin.pond.sub.org> References: <1343424728-22461-1-git-send-email-lcapitulino@redhat.com> <1343424728-22461-12-git-send-email-lcapitulino@redhat.com> <877gtihmgn.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/27] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, eblake@redhat.com, qemu-devel@nongnu.org On Wed, 01 Aug 2012 13:37:44 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > Today, hmp_cont() checks for QERR_DEVICE_ENCRYPTED in order to know > > if qmp_cont() failed due to an encrypted device. If it did, > > hmp_cont() accesses QERR_DEVICE_ENCRYPTED's data member 'device' to > > precisely know for which device an encryption key must be set. > > > > However, all errors data members are going to be dropped by a later > > commit, so hmp_cont() can't do this anymore. > > > > This commit changes hmp_cont() to loop through all block devices > > and proactively set an encryption key for any encrypted device > > without a valid one. > > > > Signed-off-by: Luiz Capitulino > > --- > > hmp.c | 43 +++++++++++++++++++++++++------------------ > > 1 file changed, 25 insertions(+), 18 deletions(-) > > > > diff --git a/hmp.c b/hmp.c > > index 6b72a64..a906f8a 100644 > > --- a/hmp.c > > +++ b/hmp.c > > @@ -610,34 +610,41 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict) > > > > static void hmp_cont_cb(void *opaque, int err) > > { > > - Monitor *mon = opaque; > > - > > if (!err) { > > - hmp_cont(mon, NULL); > > + qmp_cont(NULL); > > } > > } > > > > -void hmp_cont(Monitor *mon, const QDict *qdict) > > +static bool block_dev_is_encrypted(const BlockInfo *bdev) > > { > > - Error *errp = NULL; > > - > > - qmp_cont(&errp); > > - if (error_is_set(&errp)) { > > - if (error_is_type(errp, QERR_DEVICE_ENCRYPTED)) { > > - const char *device; > > + return (bdev->inserted && bdev->inserted->encrypted); > > +} > > > > - /* The device is encrypted. Ask the user for the password > > - and retry */ > > +static bool block_dev_key_is_set(const BlockInfo *bdev) > > +{ > > + return (bdev->inserted && bdev->inserted->valid_encryption_key); > > +} > > New static helpers block_dev_is_encrypted(), block_dev_key_is_set(). > They work on BlockInfo. Call them blockinfo_{is_encrypted,key_is_set}()? Done for v1. > > > > > - device = error_get_field(errp, "device"); > > - assert(device != NULL); > > +void hmp_cont(Monitor *mon, const QDict *qdict) > > +{ > > + BlockInfoList *bdev_list, *bdev; > > + Error *errp = NULL; > > > > - monitor_read_block_device_key(mon, device, hmp_cont_cb, mon); > > - error_free(errp); > > - return; > > + bdev_list = qmp_query_block(NULL); > > + for (bdev = bdev_list; bdev; bdev = bdev->next) { > > + if (block_dev_is_encrypted(bdev->value) && > > + !block_dev_key_is_set(bdev->value)) { > > + monitor_read_block_device_key(mon, bdev->value->device, > > + hmp_cont_cb, NULL); > > + goto out; > > Any particular reason for creating BlockInfos just to find out whether > we lack the key? Why not bdrv_key_required()? As Anthony answered in the other email, hmp.c is a real QMP client so it's only allowed to use QMP and monitor functions. > > > } > > - hmp_handle_error(mon, &errp); > > } > > + > > + qmp_cont(&errp); > > + hmp_handle_error(mon, &errp); > > + > > +out: > > + qapi_free_BlockInfoList(bdev_list); > > } > > > > void hmp_system_wakeup(Monitor *mon, const QDict *qdict) > > Diff makes this change look worse than it is. Odd: M-x ediff gets it > right. Anyway, here's how I think it works: > > Unchanged qmp_cont(): search the bdrv_states for the first encrypted one > without a key. If found, set err argument to QERR_DEVICE_ENCRYPTED. > Other errors unrelated to encrypted devices are also possible. Right, and this patch doesn't touch qmp_cont(). > hmp_cont() before: try qmp_cont(). If we get QERR_DEVICE_ENCRYPTED, > extract the device from the error object, and prompt for its key, with a > callback that retries hmp_cont() if the key was provided. Correct. > After: search the bdrv_states for an encrypted one without a key. If > there is none, qmp_cont(), no special error handling. If there is one, > prompt for its key, with a callback that runs qmp_cont() if the key was > provided. Exactly. > Have you tested this with multiple devices lacking their key? I've tested with two devices. Will test with four or five for v1.