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