[Qemu-devel] [PATCH 0/5] monitor usb: Fixes for encrypted images

[Qemu-devel] [PATCH 0/5] monitor usb: Fixes for encrypted images

[Markus Armbruster]

Fixes for encrypted images is of course a fool's errand.  My actual
motivation is killing a few more qerror_report() calls.

Markus Armbruster (5):
  monitor: Drop dead QMP check from monitor_read_password()
  monitor: Plug memory leak in monitor_read_bdrv_key_start()
  monitor usb: Inline monitor_read_bdrv_key_start()'s first part
  usb/dev-storage: Fix QMP device_add missing encryption key failure
  usb/dev-storage: Avoid qerror_report_err() outside QMP handlers

 hw/usb/dev-storage.c | 30 ++++++++++++++++++------------
 monitor.c            | 32 ++++++++++++--------------------
 2 files changed, 30 insertions(+), 32 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/5] monitor: Drop dead QMP check from monitor_read_password()

[Markus Armbruster]

Function is only called in HMP context since commit 333a96e "qapi:
Convert change".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index c86a89e..29c930a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -265,10 +265,7 @@ void monitor_read_command(Monitor *mon, int show_prompt)
 int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
                           void *opaque)
 {
-    if (monitor_ctrl_mode(mon)) {
-        qerror_report(QERR_MISSING_PARAMETER, "password");
-        return -EINVAL;
-    } else if (mon->rs) {
+    if (mon->rs) {
         readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
         /* prompt is printed on return from the command handler */
         return 0;
-- 
1.9.3

[Qemu-devel] [PATCH 2/5] monitor: Plug memory leak in monitor_read_bdrv_key_start()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/monitor.c b/monitor.c
index 29c930a..12d80a1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5390,9 +5390,11 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
 
     if (monitor_ctrl_mode(mon)) {
         qerror_report_err(local_err);
+        error_free(local_err);
         return -1;
     }
 
+    error_free(local_err);
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
                    bdrv_get_encrypted_filename(bs));
 
-- 
1.9.3

[Qemu-devel] [PATCH 3/5] monitor usb: Inline monitor_read_bdrv_key_start()'s first part

[Markus Armbruster]

monitor_read_bdrv_key_start() does several things:

1. If no key is needed, call completion_cb() and succeed

2. If we're in QMP context, call qerror_report_err() and fail

3. Start reading the key in the monitor.

This is two things too many.  Inline 1. and 2. into its callers
monitor_read_block_device_key() and usb_msd_realize_storage().

Since monitor_read_block_device_key() only ever runs in HMP context,
drop 2. there.

The next commit will clean up the result in usb_msd_realize_storage().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/dev-storage.c | 13 +++++++++++--
 monitor.c            | 29 +++++++++++------------------
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index dacefd7..f47c856 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -641,8 +641,17 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
 
     if (bdrv_key_required(blk_bs(blk))) {
         if (cur_mon) {
-            monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
-                                        usb_msd_password_cb, s);
+            bdrv_add_key(blk_bs(blk), NULL, &err);
+            if (!err) {
+                usb_msd_password_cb(s, 0);
+            } else if (monitor_cur_is_qmp()) {
+                qerror_report_err(err);
+                error_free(err);
+            } else {
+                error_free(err);
+                monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
+                                            usb_msd_password_cb, s);
+            }
             s->dev.auto_attach = 0;
         } else {
             autostart = 0;
diff --git a/monitor.c b/monitor.c
index 12d80a1..85223cf 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5376,25 +5376,8 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockCompletionFunc *completion_cb,
                                 void *opaque)
 {
-    Error *local_err = NULL;
     int err;
 
-    bdrv_add_key(bs, NULL, &local_err);
-    if (!local_err) {
-        if (completion_cb)
-            completion_cb(opaque, 0);
-        return 0;
-    }
-
-    /* Need a key for @bs */
-
-    if (monitor_ctrl_mode(mon)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        return -1;
-    }
-
-    error_free(local_err);
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
                    bdrv_get_encrypted_filename(bs));
 
@@ -5413,6 +5396,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   BlockCompletionFunc *completion_cb,
                                   void *opaque)
 {
+    Error *err = NULL;
     BlockDriverState *bs;
 
     bs = bdrv_find(device);
@@ -5421,7 +5405,16 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
         return -1;
     }
 
-    return monitor_read_bdrv_key_start(mon, bs, completion_cb, opaque);
+    bdrv_add_key(bs, NULL, &err);
+    if (err) {
+        error_free(err);
+        return monitor_read_bdrv_key_start(mon, bs, completion_cb, opaque);
+    }
+
+    if (completion_cb) {
+        completion_cb(opaque, 0);
+    }
+    return 0;
 }
 
 QemuOptsList qemu_mon_opts = {
-- 
1.9.3

[Qemu-devel] [PATCH 4/5] usb/dev-storage: Fix QMP device_add missing encryption key failure

[Markus Armbruster]

When the image is encrypted, QMP device_add creates the device, defers
actually attaching it to when the key becomes available, then returns
an error.  This is wrong.  device_add must either create the device
and succeed, or do nothing and fail.

The bug is in usb_msd_realize_storage().  It posts an error with
qerror_report_err(), and returns success.  Device realization relies
on the return value, and completes.  The QMP monitor, however, relies
on the posted error, and sends it in an error reply.

Reproducer:

    $ qemu-system-x86_64 -nodefaults -display none -usb -qmp stdio -drive if=none,id=foo,file=geheim.qcow2
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
    { "execute": "qmp_capabilities" }
    {"return": {}}
    { "execute": "device_add", "arguments": { "driver": "usb-storage", "id": "bar", "drive": "foo" } }
    {"error": {"class": "DeviceEncrypted", "desc": "'foo' (geheim.qcow2) is encrypted"}}

Even though we got an error back, the device got created just fine.
To demonstrate, let's unplug it again:

    {"execute":"device_del","arguments": { "id": "bar" } }
    {"timestamp": {"seconds": 1426003440, "microseconds": 237181}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/bar/bar.0/legacy[0]"}}
    {"timestamp": {"seconds": 1426003440, "microseconds": 238231}, "event": "DEVICE_DELETED", "data": {"device": "bar", "path": "/machine/peripheral/bar"}}
    {"return": {}}

Fix by making usb_msd_realize_storage() fail properly.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/dev-storage.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index f47c856..f50bcb8 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -610,6 +610,23 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
         return;
     }
 
+    bdrv_add_key(blk_bs(blk), NULL, &err);
+    if (err) {
+        if (monitor_cur_is_qmp()) {
+            error_propagate(errp, err);
+            return;
+        }
+        error_free(err);
+        err = NULL;
+        if (cur_mon) {
+            monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
+                                        usb_msd_password_cb, s);
+            s->dev.auto_attach = 0;
+        } else {
+            autostart = 0;
+        }
+    }
+
     blkconf_serial(&s->conf, &dev->serial);
     blkconf_blocksizes(&s->conf);
 
@@ -638,25 +655,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
     }
     usb_msd_handle_reset(dev);
     s->scsi_dev = scsi_dev;
-
-    if (bdrv_key_required(blk_bs(blk))) {
-        if (cur_mon) {
-            bdrv_add_key(blk_bs(blk), NULL, &err);
-            if (!err) {
-                usb_msd_password_cb(s, 0);
-            } else if (monitor_cur_is_qmp()) {
-                qerror_report_err(err);
-                error_free(err);
-            } else {
-                error_free(err);
-                monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
-                                            usb_msd_password_cb, s);
-            }
-            s->dev.auto_attach = 0;
-        } else {
-            autostart = 0;
-        }
-    }
 }
 
 static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
-- 
1.9.3

[Qemu-devel] [PATCH 5/5] usb/dev-storage: Avoid qerror_report_err() outside QMP handlers

[Markus Armbruster]

qerror_report_err() is a transitional interface to help with
converting existing monitor commands to QMP.  It should not be used
elsewhere.

usb_msd_password_cb() is only called from within an HMP command
handler.  Replace by error_report_err().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/dev-storage.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index f50bcb8..ae8d40d 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -559,8 +559,7 @@ static void usb_msd_password_cb(void *opaque, int err)
     }
 
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_report_err(local_err);
         qdev_unplug(&s->dev.qdev, NULL);
     }
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/5] monitor usb: Fixes for encrypted images

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

On 03/12/2015 10:26 AM, Markus Armbruster wrote:
> Fixes for encrypted images is of course a fool's errand.  My actual
> motivation is killing a few more qerror_report() calls.
> 
> Markus Armbruster (5):
>   monitor: Drop dead QMP check from monitor_read_password()
>   monitor: Plug memory leak in monitor_read_bdrv_key_start()
>   monitor usb: Inline monitor_read_bdrv_key_start()'s first part
>   usb/dev-storage: Fix QMP device_add missing encryption key failure
>   usb/dev-storage: Avoid qerror_report_err() outside QMP handlers

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  hw/usb/dev-storage.c | 30 ++++++++++++++++++------------
>  monitor.c            | 32 ++++++++++++--------------------
>  2 files changed, 30 insertions(+), 32 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/5] monitor usb: Fixes for encrypted images

[Gerd Hoffmann]

On Do, 2015-03-12 at 17:26 +0100, Markus Armbruster wrote:
> Fixes for encrypted images is of course a fool's errand.  My actual
> motivation is killing a few more qerror_report() calls.
> 
> Markus Armbruster (5):
>   monitor: Drop dead QMP check from monitor_read_password()
>   monitor: Plug memory leak in monitor_read_bdrv_key_start()
>   monitor usb: Inline monitor_read_bdrv_key_start()'s first part
>   usb/dev-storage: Fix QMP device_add missing encryption key failure
>   usb/dev-storage: Avoid qerror_report_err() outside QMP handlers

Added to usb patch queue.

thanks,
  Gerd