Patchwork [12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 2, 2012, 1:02 a.m.
Message ID <1343869374-23417-13-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174633/
State New
Headers show

Comments

Luiz Capitulino - Aug. 2, 2012, 1:02 a.m.
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(-)
Markus Armbruster - Aug. 2, 2012, 11:53 a.m.
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.

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?

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.
Luiz Capitulino - Aug. 2, 2012, 2:22 p.m.
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.
Markus Armbruster - Aug. 10, 2012, 8:42 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

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

Ugh.  Clunky even for QEMU standards.

cont gives no indication that the run state change didn't happen.

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

Yes.  Let's create an encrypted QCOW2 image without backing_file:

    $ qemu-img create -f qcow2 -o encryption,size=1G foo.qcow2
    Formatting 'foo.qcow2', fmt=qcow2 size=1073741824 encryption=on cluster_size=65536 lazy_refcounts=off 
    $ qemu-img info foo.qcow2
    Disk image 'foo.qcow2' is encrypted.
    password: 
    image: foo.qcow2
    file format: qcow2
    virtual size: 1.0G (1073741824 bytes)
    disk size: 136K
    encrypted: yes
    cluster_size: 65536
    $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb -monitor stdio -drive if=ide,file=foo.qcow2,id=foo
    QEMU 1.1.50 monitor - type 'help' for more information
    (qemu) info block
    foo: removable=0 io-status=ok file=foo.qcow2 ro=0 drv=qcow2 encrypted=1 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
    (qemu) c
    foo (foo.qcow2) is encrypted.
    Password: 

Now wrap an unencrypted one around it:

    $ qemu-img create -f qcow2 -o size=1G,backing_file=foo.qcow2 bar.qcow2
    Formatting 'bar.qcow2', fmt=qcow2 size=1073741824 backing_file='foo.qcow2' encryption=off cluster_size=65536 lazy_refcounts=off 
    $ qemu-img info bar.qcow2
    image: bar.qcow2
    file format: qcow2
    virtual size: 1.0G (1073741824 bytes)
    disk size: 196K
    cluster_size: 65536
    backing file: foo.qcow2
    $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb -monitor stdio -drive if=ide,file=bar.qcow2,id=foo
    QEMU 1.1.50 monitor - type 'help' for more information
    (qemu) c
    'foo' (foo.qcow2) is encrypted
    (qemu) 

Regression :)

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

Another way is to replace valid-encryption-key by the predicate that's
actually wanted: key-required.
Luiz Capitulino - Aug. 10, 2012, 2:22 p.m.
On Fri, 10 Aug 2012 10:42:23 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > 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.
> 
> Ugh.  Clunky even for QEMU standards.
> 
> cont gives no indication that the run state change didn't happen.

Agreed. We should have freedom to change cont's semantics on HMP if we
need/want to in order to fix this.

But that's out of the scope of this series, of course.

> 
> >> 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?
> 
> Yes.  Let's create an encrypted QCOW2 image without backing_file:
> 
>     $ qemu-img create -f qcow2 -o encryption,size=1G foo.qcow2
>     Formatting 'foo.qcow2', fmt=qcow2 size=1073741824 encryption=on cluster_size=65536 lazy_refcounts=off 
>     $ qemu-img info foo.qcow2
>     Disk image 'foo.qcow2' is encrypted.
>     password: 
>     image: foo.qcow2
>     file format: qcow2
>     virtual size: 1.0G (1073741824 bytes)
>     disk size: 136K
>     encrypted: yes
>     cluster_size: 65536
>     $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb -monitor stdio -drive if=ide,file=foo.qcow2,id=foo
>     QEMU 1.1.50 monitor - type 'help' for more information
>     (qemu) info block
>     foo: removable=0 io-status=ok file=foo.qcow2 ro=0 drv=qcow2 encrypted=1 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
>     (qemu) c
>     foo (foo.qcow2) is encrypted.
>     Password: 
> 
> Now wrap an unencrypted one around it:
> 
>     $ qemu-img create -f qcow2 -o size=1G,backing_file=foo.qcow2 bar.qcow2
>     Formatting 'bar.qcow2', fmt=qcow2 size=1073741824 backing_file='foo.qcow2' encryption=off cluster_size=65536 lazy_refcounts=off 
>     $ qemu-img info bar.qcow2
>     image: bar.qcow2
>     file format: qcow2
>     virtual size: 1.0G (1073741824 bytes)
>     disk size: 196K
>     cluster_size: 65536
>     backing file: foo.qcow2
>     $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0 -usb -monitor stdio -drive if=ide,file=bar.qcow2,id=foo
>     QEMU 1.1.50 monitor - type 'help' for more information
>     (qemu) c
>     'foo' (foo.qcow2) is encrypted
>     (qemu) 
> 
> Regression :)

Hmm, right. I think this can also be reproduced by passing -snapshot
when using an encrypted image.

> >> 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.
> 
> Another way is to replace valid-encryption-key by the predicate that's
> actually wanted: key-required.

Looks good, this would fix the bug above too, right?
Markus Armbruster - Aug. 10, 2012, 4:37 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 10 Aug 2012 10:42:23 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > 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.
>> 
>> Ugh.  Clunky even for QEMU standards.
>> 
>> cont gives no indication that the run state change didn't happen.
>
> Agreed. We should have freedom to change cont's semantics on HMP if we
> need/want to in order to fix this.
>
> But that's out of the scope of this series, of course.

Yup.

>> >> 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?
>> 
>> Yes.  Let's create an encrypted QCOW2 image without backing_file:
>> 
>>     $ qemu-img create -f qcow2 -o encryption,size=1G foo.qcow2
>>     Formatting 'foo.qcow2', fmt=qcow2 size=1073741824 encryption=on
>> cluster_size=65536 lazy_refcounts=off
>>     $ qemu-img info foo.qcow2
>>     Disk image 'foo.qcow2' is encrypted.
>>     password: 
>>     image: foo.qcow2
>>     file format: qcow2
>>     virtual size: 1.0G (1073741824 bytes)
>>     disk size: 136K
>>     encrypted: yes
>>     cluster_size: 65536
>>     $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0
>> -usb -monitor stdio -drive if=ide,file=foo.qcow2,id=foo
>>     QEMU 1.1.50 monitor - type 'help' for more information
>>     (qemu) info block
>>     foo: removable=0 io-status=ok file=foo.qcow2 ro=0 drv=qcow2
>> encrypted=1 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
>>     (qemu) c
>>     foo (foo.qcow2) is encrypted.
>>     Password: 
>> 
>> Now wrap an unencrypted one around it:
>> 
>>     $ qemu-img create -f qcow2 -o size=1G,backing_file=foo.qcow2 bar.qcow2
>>     Formatting 'bar.qcow2', fmt=qcow2 size=1073741824
>> backing_file='foo.qcow2' encryption=off cluster_size=65536
>> lazy_refcounts=off
>>     $ qemu-img info bar.qcow2
>>     image: bar.qcow2
>>     file format: qcow2
>>     virtual size: 1.0G (1073741824 bytes)
>>     disk size: 196K
>>     cluster_size: 65536
>>     backing file: foo.qcow2
>>     $ qemu-system-x86_64 -nodefaults --enable-kvm -S -m 512 -vnc :0
>> -usb -monitor stdio -drive if=ide,file=bar.qcow2,id=foo
>>     QEMU 1.1.50 monitor - type 'help' for more information
>>     (qemu) c
>>     'foo' (foo.qcow2) is encrypted
>>     (qemu) 
>> 
>> Regression :)
>
> Hmm, right. I think this can also be reproduced by passing -snapshot
> when using an encrypted image.
>
>> >> 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.
>> 
>> Another way is to replace valid-encryption-key by the predicate that's
>> actually wanted: key-required.
>
> Looks good, this would fix the bug above too, right?

The one I marked "Regression :)"?  Yes, I think it would fix that one.

Patch

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)