| Message ID | 1343424728-22461-12-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > Today, hmp_cont() checks for QERR_DEVICE_ENCRYPTED in order to know > if qmp_cont() failed due to an encrypted device. If it did, > hmp_cont() accesses QERR_DEVICE_ENCRYPTED's data member 'device' to > precisely know for which device an encryption key must be set. > > However, all errors data members are going to be dropped by a later > commit, so hmp_cont() can't do this anymore. > > 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. > > 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..a906f8a 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 block_dev_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 block_dev_key_is_set(const BlockInfo *bdev) > +{ > + return (bdev->inserted && bdev->inserted->valid_encryption_key); > +} New static helpers block_dev_is_encrypted(), block_dev_key_is_set(). They work on BlockInfo. Call them blockinfo_{is_encrypted,key_is_set}()? > > - 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 (block_dev_is_encrypted(bdev->value) && > + !block_dev_key_is_set(bdev->value)) { > + monitor_read_block_device_key(mon, bdev->value->device, > + hmp_cont_cb, NULL); > + goto out; Any particular reason for creating BlockInfos just to find out whether we lack the key? Why not bdrv_key_required()? > } > - 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) 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. Have you tested this with multiple devices lacking their key?
On Wed, 01 Aug 2012 13:37:44 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > Today, hmp_cont() checks for QERR_DEVICE_ENCRYPTED in order to know > > if qmp_cont() failed due to an encrypted device. If it did, > > hmp_cont() accesses QERR_DEVICE_ENCRYPTED's data member 'device' to > > precisely know for which device an encryption key must be set. > > > > However, all errors data members are going to be dropped by a later > > commit, so hmp_cont() can't do this anymore. > > > > 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. > > > > 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..a906f8a 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 block_dev_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 block_dev_key_is_set(const BlockInfo *bdev) > > +{ > > + return (bdev->inserted && bdev->inserted->valid_encryption_key); > > +} > > New static helpers block_dev_is_encrypted(), block_dev_key_is_set(). > They work on BlockInfo. Call them blockinfo_{is_encrypted,key_is_set}()? Done for v1. > > > > > - 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 (block_dev_is_encrypted(bdev->value) && > > + !block_dev_key_is_set(bdev->value)) { > > + monitor_read_block_device_key(mon, bdev->value->device, > > + hmp_cont_cb, NULL); > > + goto out; > > Any particular reason for creating BlockInfos just to find out whether > we lack the key? Why not bdrv_key_required()? As Anthony answered in the other email, hmp.c is a real QMP client so it's only allowed to use QMP and monitor functions. > > > } > > - 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) > > 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. Right, and this patch doesn't touch qmp_cont(). > 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. Correct. > 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. Exactly. > Have you tested this with multiple devices lacking their key? I've tested with two devices. Will test with four or five for v1.
diff --git a/hmp.c b/hmp.c index 6b72a64..a906f8a 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 block_dev_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 block_dev_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 (block_dev_is_encrypted(bdev->value) && + !block_dev_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)
Today, hmp_cont() checks for QERR_DEVICE_ENCRYPTED in order to know if qmp_cont() failed due to an encrypted device. If it did, hmp_cont() accesses QERR_DEVICE_ENCRYPTED's data member 'device' to precisely know for which device an encryption key must be set. However, all errors data members are going to be dropped by a later commit, so hmp_cont() can't do this anymore. 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. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- hmp.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)