Patchwork [11/27] hmp: hmp_cont(): don't rely on QERR_DEVICE_ENCRYPTED

login
register
mail settings
Submitter Luiz Capitulino
Date July 27, 2012, 9:31 p.m.
Message ID <1343424728-22461-12-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/173805/
State New
Headers show

Comments

Luiz Capitulino - July 27, 2012, 9:31 p.m.
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(-)
Markus Armbruster - Aug. 1, 2012, 11:37 a.m.
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?
Luiz Capitulino - Aug. 1, 2012, 1:47 p.m.
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.

Patch

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)