Patchwork [13/35] hmp_change(): don't access DeviceEncrypted's data

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 7, 2012, 3:53 p.m.
Message ID <1344354826-10375-14-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/175748/
State New
Headers show

Comments

Luiz Capitulino - Aug. 7, 2012, 3:53 p.m.
It's not needed. The device name is already known and
monitor_read_block_device_key() knows how to do the rest. This overly
simplifies hmp_change().

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hmp.c | 29 ++---------------------------
 1 file changed, 2 insertions(+), 27 deletions(-)
Markus Armbruster - Aug. 10, 2012, 9:02 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> It's not needed. The device name is already known and
> monitor_read_block_device_key() knows how to do the rest. This overly
> simplifies hmp_change().

"overly"?

My usual complaint about commit messages is that they fail to explain
the change's purpose.  Yours explains your reason just fine, but the
description of what's done falls a bit short.  I'd like to see something
like "replace duplicated password prompting code by common
monitor_read_block_device_key()".
Luiz Capitulino - Aug. 10, 2012, 2:36 p.m.
On Fri, 10 Aug 2012 11:02:21 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > It's not needed. The device name is already known and
> > monitor_read_block_device_key() knows how to do the rest. This overly
> > simplifies hmp_change().
> 
> "overly"?
> 
> My usual complaint about commit messages is that they fail to explain
> the change's purpose.  Yours explains your reason just fine, but the
> description of what's done falls a bit short.  I'd like to see something
> like "replace duplicated password prompting code by common
> monitor_read_block_device_key()".

Done, for v3.

Patch

diff --git a/hmp.c b/hmp.c
index bfcc032..3a9688d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -782,22 +782,6 @@  static void hmp_change_read_arg(Monitor *mon, const char *password,
     monitor_read_command(mon, 1);
 }
 
-static void cb_hmp_change_bdrv_pwd(Monitor *mon, const char *password,
-                                   void *opaque)
-{
-    Error *encryption_err = opaque;
-    Error *err = NULL;
-    const char *device;
-
-    device = error_get_field(encryption_err, "device");
-
-    qmp_block_passwd(device, password, &err);
-    hmp_handle_error(mon, &err);
-    error_free(encryption_err);
-
-    monitor_read_command(mon, 1);
-}
-
 void hmp_change(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -816,17 +800,8 @@  void hmp_change(Monitor *mon, const QDict *qdict)
 
     qmp_change(device, target, !!arg, arg, &err);
     if (error_is_type(err, QERR_DEVICE_ENCRYPTED)) {
-        monitor_printf(mon, "%s (%s) is encrypted.\n",
-                       error_get_field(err, "device"),
-                       error_get_field(err, "filename"));
-        if (!monitor_get_rs(mon)) {
-            monitor_printf(mon,
-                    "terminal does not support password prompting\n");
-            error_free(err);
-            return;
-        }
-        readline_start(monitor_get_rs(mon), "Password: ", 1,
-                       cb_hmp_change_bdrv_pwd, err);
+        error_free(err);
+        monitor_read_block_device_key(mon, device, NULL, NULL);
         return;
     }
     hmp_handle_error(mon, &err);