Luiz Capitulino <lcapitul...@redhat.com> writes: > On Thu, 02 Aug 2012 13:53:08 +0200 > Markus Armbruster <arm...@redhat.com> wrote: > >> Luiz Capitulino <lcapitul...@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 <lcapitul...@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.