| Message ID | 1343869374-23417-13-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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?
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.
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)
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(-)