Re: [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 02 Aug 2012 13:53:08 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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.
> >
> > This change is needed because QERR_DEVICE_ENCRYPTED is going to be
> > dropped by a future commit.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hmp.c | 43 +++++++++++++++++++++++++------------------
> >  1 file changed, 25 insertions(+), 18 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 6b72a64..1ebeb63 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 blockinfo_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 blockinfo_key_is_set(const BlockInfo *bdev)
> > +{
> > +    return (bdev->inserted && bdev->inserted->valid_encryption_key);
> > +}
> >  
> > -            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 (blockinfo_is_encrypted(bdev->value) &&
> > +            !blockinfo_key_is_set(bdev->value)) {
> > +                monitor_read_block_device_key(mon, bdev->value->device,
> > +                                              hmp_cont_cb, NULL);
> > +                goto out;
> >          }
> > -        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)
> 
> Quote my previous analysis:
> 
> 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.
> 
> 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.
> 
> 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.
> 
> End quote.
> 
> Two observations:
> 
> 1. I don't understand how this works for multiple encrypted BDSs without
> keys.  If there are any, hmp_cont() starts reading the first one's key,
> then returns.  But the callback doesn't start reading the next one's
> key.  Please explain.

The callback calls qmp_cont(), which will fail. Then the user will enter
cont again, and the loop on BlockInfos will run again and the user will
be asked for the password of the next image.

IOW, each time cont is issued by the user it will ask for the password
of a different device.

That's the current behavior, and I believe it was also the behavior before
I converted cont to the qapi.

> 2. qmp_cont() uses bdrv_key_required() to test whether a BDS lacks a
> key.  Your new hmp_cont() uses blockinfo_is_encrypted() &&
> !blockinfo_key_is_set().  Not obvious that the two are equivalent.
> 
> I'm afraid they are not.  bdrv_key_required() checks the backing image
> first:
> 
>     int bdrv_key_required(BlockDriverState *bs)
>     {
>         BlockDriverState *backing_hd = bs->backing_hd;
> 
>         if (backing_hd && backing_hd->encrypted && !backing_hd->valid_key)
>             return 1;
>         return (bs->encrypted && !bs->valid_key);
>     }
> 
> Your code doesn't:
> 
>     static bool blockinfo_is_encrypted(const BlockInfo *bdev)
>     {
>         return (bdev->inserted && bdev->inserted->encrypted);
>     }
> 
>     static bool blockinfo_key_is_set(const BlockInfo *bdev)
>     {
>         return (bdev->inserted && bdev->inserted->valid_encryption_key);
>     }
> 
>     BlockInfoList *qmp_query_block(Error **errp)
>     {
>         BlockInfoList *head = NULL, *cur_item = NULL;
>         BlockDriverState *bs;
> 
>         QTAILQ_FOREACH(bs, &bdrv_states, list) {
>             BlockInfoList *info = g_malloc0(sizeof(*info));
> [...]
>             if (bs->drv) {
>                 info->value->has_inserted = true;
>                 info->value->inserted = g_malloc0(sizeof(*info->value->inserted));
> [...]
>                 info->value->inserted->encrypted = bs->encrypted;
>                 info->value->inserted->valid_encryption_key = bs->valid_key;
> [...]
> 
> Are you sure this is correct?

Is it actually possible for backing_hd to be false and valid_key to be true?

> I understand we require HMP code to go via QMP for everything, to keep
> HMP honest.  This case shows a drawback: duplicated code, leading to
> inconsistencies.

Keeping DeviceEncrypted would solve this.