[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVIC
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED |
Date: |
Fri, 10 Aug 2012 10:42:23 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Luiz Capitulino <address@hidden> writes:
> On Thu, 02 Aug 2012 13:53:08 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> 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 <address@hidden>
>> > ---
>> > 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.
- [Qemu-devel] [PATCH 06/34] qerror: avoid passing qerr pointer, (continued)
- [Qemu-devel] [PATCH 06/34] qerror: avoid passing qerr pointer, Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 05/34] qerror: drop qerror_abort(), Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 08/34] qerror: qerror_format(): return an allocated string, Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 07/34] qerror: QError: drop file, linenr, func, Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 10/34] error: don't delay error message construction, Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED, Luiz Capitulino, 2012/08/01
[Qemu-devel] [PATCH 14/34] net: inet_connect(), inet_connect_opts(): add in_progress argument, Luiz Capitulino, 2012/08/01
[Qemu-devel] [PATCH 11/34] qmp: query-block: add 'valid_encryption_key' field, Luiz Capitulino, 2012/08/01