[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: |
Thu, 02 Aug 2012 13:53:08 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
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.
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.
- [Qemu-devel] [PATCH 03/34] qerror: QERR_DEVICE_ENCRYPTED: add filename info to human msg, (continued)
- [Qemu-devel] [PATCH 03/34] qerror: QERR_DEVICE_ENCRYPTED: add filename info to human msg, Luiz Capitulino, 2012/08/01
- [Qemu-devel] [PATCH 04/34] qerror: reduce public exposure, Luiz Capitulino, 2012/08/01
- [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
- Re: [Qemu-devel] [PATCH 12/34] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED,
Markus Armbruster <=
[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