On Wed, 01 Aug 2012 13:37:44 +0200
Markus Armbruster <arm...@redhat.com> wrote:
> Luiz Capitulino <lcapitul...@redhat.com> 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 <lcapitul...@redhat.com>
> > ---
> > 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.
