[Qemu-devel] [PATCH v2 0/4] block: Cleanups around error reporting

[Qemu-devel] [PATCH v2 0/4] block: Cleanups around error reporting

[Markus Armbruster]

v2:
* PATCH 4: Use error_setg() [Kevin]
* R-bys retained

Markus Armbruster (4):
  blockdev: Give find_block_job() an Error ** parameter
  blockdev: Eliminate silly QERR_BLOCK_JOB_NOT_ACTIVE macro
  block: New bdrv_add_key(), convert monitor to use it
  block: Eliminate silly QERR_ macros used for encryption keys

 block.c                   | 30 ++++++++++++++++++++++++++++++
 blockdev.c                | 44 +++++++++++---------------------------------
 include/block/block.h     |  1 +
 include/qapi/qmp/qerror.h |  9 ---------
 monitor.c                 | 16 +++++++++++-----
 qmp.c                     |  8 ++++----
 6 files changed, 57 insertions(+), 51 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/4] blockdev: Give find_block_job() an Error ** parameter

[Markus Armbruster]

When find_block_job() fails, all its callers build the same Error
object.  Build it in find_block_job() instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d59efd3..8d6ca35 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2653,7 +2653,8 @@ out:
 }
 
 /* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context)
+static BlockJob *find_block_job(const char *device, AioContext **aio_context,
+                                Error **errp)
 {
     BlockDriverState *bs;
 
@@ -2673,6 +2674,7 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context)
     return bs->job;
 
 notfound:
+    error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
     *aio_context = NULL;
     return NULL;
 }
@@ -2680,10 +2682,9 @@ notfound:
 void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context);
+    BlockJob *job = find_block_job(device, &aio_context, errp);
 
     if (!job) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
 
@@ -2695,10 +2696,9 @@ void qmp_block_job_cancel(const char *device,
                           bool has_force, bool force, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context);
+    BlockJob *job = find_block_job(device, &aio_context, errp);
 
     if (!job) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
 
@@ -2721,10 +2721,9 @@ out:
 void qmp_block_job_pause(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context);
+    BlockJob *job = find_block_job(device, &aio_context, errp);
 
     if (!job) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
 
@@ -2736,10 +2735,9 @@ void qmp_block_job_pause(const char *device, Error **errp)
 void qmp_block_job_resume(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context);
+    BlockJob *job = find_block_job(device, &aio_context, errp);
 
     if (!job) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
 
@@ -2751,10 +2749,9 @@ void qmp_block_job_resume(const char *device, Error **errp)
 void qmp_block_job_complete(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context);
+    BlockJob *job = find_block_job(device, &aio_context, errp);
 
     if (!job) {
-        error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
         return;
     }
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 1/4] blockdev: Give find_block_job() an Error ** parameter

[Max Reitz]

On 2015-01-29 at 04:36, Markus Armbruster wrote:
> When find_block_job() fails, all its callers build the same Error
> object.  Build it in find_block_job() instead.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   blockdev.c | 19 ++++++++-----------
>   1 file changed, 8 insertions(+), 11 deletions(-)

I think I'd like it better to return "Device not found" in case 
bdrv_find() fails, but that's probably a matter of taste:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/4] blockdev: Give find_block_job() an Error ** parameter

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> On 2015-01-29 at 04:36, Markus Armbruster wrote:
>> When find_block_job() fails, all its callers build the same Error
>> object.  Build it in find_block_job() instead.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   blockdev.c | 19 ++++++++-----------
>>   1 file changed, 8 insertions(+), 11 deletions(-)
>
> I think I'd like it better to return "Device not found" in case
> bdrv_find() fails, but that's probably a matter of taste:

Could be done on top.  This patch intentionally doesn't change behavior.

Of course, the whole job thing needs a redesign to liberate it from its
tie to a single BDS.  And then we won't bdrv_find() to find jobs.

> Reviewed-by: Max Reitz <mreitz@redhat.com>

Thanks!

[Qemu-devel] [PATCH v2 2/4] blockdev: Eliminate silly QERR_BLOCK_JOB_NOT_ACTIVE macro

[Markus Armbruster]

From: Markus Armbruster <armbru@pond.sub.org>

The QERR_ macros are leftovers from the days of "rich" error objects.
They're used with error_set() and qerror_report(), and expand into the
first *two* arguments.  This trickiness has become pointless.  Clean
this one up.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c                | 3 ++-
 include/qapi/qmp/qerror.h | 3 ---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8d6ca35..287d7af 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2674,7 +2674,8 @@ static BlockJob *find_block_job(const char *device, AioContext **aio_context,
     return bs->job;
 
 notfound:
-    error_set(errp, QERR_BLOCK_JOB_NOT_ACTIVE, device);
+    error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
+              "No active block job on device '%s'", device);
     *aio_context = NULL;
     return NULL;
 }
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 0ca6cbd..becdca6 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -37,9 +37,6 @@ void qerror_report_err(Error *err);
 #define QERR_BASE_NOT_FOUND \
     ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
 
-#define QERR_BLOCK_JOB_NOT_ACTIVE \
-    ERROR_CLASS_DEVICE_NOT_ACTIVE, "No active block job on device '%s'"
-
 #define QERR_BLOCK_JOB_NOT_READY \
     ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be completed"
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/4] blockdev: Eliminate silly QERR_BLOCK_JOB_NOT_ACTIVE macro

[Max Reitz]

On 2015-01-29 at 04:36, Markus Armbruster wrote:
> From: Markus Armbruster <armbru@pond.sub.org>

Intentional?

> The QERR_ macros are leftovers from the days of "rich" error objects.
> They're used with error_set() and qerror_report(), and expand into the
> first *two* arguments.  This trickiness has become pointless.  Clean
> this one up.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   blockdev.c                | 3 ++-
>   include/qapi/qmp/qerror.h | 3 ---
>   2 files changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 3/4] block: New bdrv_add_key(), convert monitor to use it

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               | 29 +++++++++++++++++++++++++++++
 blockdev.c            | 24 ++----------------------
 include/block/block.h |  1 +
 monitor.c             | 16 +++++++++++-----
 qmp.c                 |  8 ++++----
 5 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/block.c b/block.c
index d45e4dd..aa12160 100644
--- a/block.c
+++ b/block.c
@@ -3719,6 +3719,35 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
     return ret;
 }
 
+/*
+ * Provide an encryption key for @bs.
+ * If @key is non-null:
+ *     If @bs is not encrypted, fail.
+ *     Else if the key is invalid, fail.
+ *     Else set @bs's key to @key, replacing the existing key, if any.
+ * If @key is null:
+ *     If @bs is encrypted and still lacks a key, fail.
+ *     Else do nothing.
+ * On failure, store an error object through @errp if non-null.
+ */
+void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
+{
+    if (key) {
+        if (!bdrv_is_encrypted(bs)) {
+            error_set(errp, QERR_DEVICE_NOT_ENCRYPTED,
+                      bdrv_get_device_name(bs));
+        } else if (bdrv_set_key(bs, key) < 0) {
+            error_set(errp, QERR_INVALID_PASSWORD);
+        }
+    } else {
+        if (bdrv_key_required(bs)) {
+            error_set(errp, QERR_DEVICE_ENCRYPTED,
+                      bdrv_get_device_name(bs),
+                      bdrv_get_encrypted_filename(bs));
+        }
+    }
+}
+
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
     return bs->drv ? bs->drv->format_name : NULL;
diff --git a/blockdev.c b/blockdev.c
index 287d7af..7d34960 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1793,7 +1793,6 @@ void qmp_block_passwd(bool has_device, const char *device,
     Error *local_err = NULL;
     BlockDriverState *bs;
     AioContext *aio_context;
-    int err;
 
     bs = bdrv_lookup_bs(has_device ? device : NULL,
                         has_node_name ? node_name : NULL,
@@ -1806,16 +1805,8 @@ void qmp_block_passwd(bool has_device, const char *device,
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    err = bdrv_set_key(bs, password);
-    if (err == -EINVAL) {
-        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
-        goto out;
-    } else if (err < 0) {
-        error_set(errp, QERR_INVALID_PASSWORD);
-        goto out;
-    }
+    bdrv_add_key(bs, password, errp);
 
-out:
     aio_context_release(aio_context);
 }
 
@@ -1833,18 +1824,7 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
         return;
     }
 
-    if (bdrv_key_required(bs)) {
-        if (password) {
-            if (bdrv_set_key(bs, password) < 0) {
-                error_set(errp, QERR_INVALID_PASSWORD);
-            }
-        } else {
-            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
-        }
-    } else if (password) {
-        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
-    }
+    bdrv_add_key(bs, password, errp);
 }
 
 void qmp_change_blockdev(const char *device, const char *filename,
diff --git a/include/block/block.h b/include/block/block.h
index 3082d2b..623c390 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -378,6 +378,7 @@ BlockDriverState *bdrv_next(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
 int bdrv_set_key(BlockDriverState *bs, const char *key);
+void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp);
 int bdrv_query_missing_keys(void);
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
                          void *opaque);
diff --git a/monitor.c b/monitor.c
index 7e4f605..50dceac 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5366,9 +5366,12 @@ static void bdrv_password_cb(void *opaque, const char *password,
     Monitor *mon = opaque;
     BlockDriverState *bs = readline_opaque;
     int ret = 0;
+    Error *local_err = NULL;
 
-    if (bdrv_set_key(bs, password) != 0) {
-        monitor_printf(mon, "invalid password\n");
+    bdrv_add_key(bs, password, &local_err);
+    if (local_err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
         ret = -EPERM;
     }
     if (mon->password_completion_cb)
@@ -5386,17 +5389,20 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockCompletionFunc *completion_cb,
                                 void *opaque)
 {
+    Error *local_err = NULL;
     int err;
 
-    if (!bdrv_key_required(bs)) {
+    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(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+        qerror_report_err(local_err);
         return -1;
     }
 
diff --git a/qmp.c b/qmp.c
index 963305c..b5a4b6e 100644
--- a/qmp.c
+++ b/qmp.c
@@ -150,6 +150,7 @@ SpiceInfo *qmp_query_spice(Error **errp)
 
 void qmp_cont(Error **errp)
 {
+    Error *local_err = NULL;
     BlockDriverState *bs;
 
     if (runstate_needs_reset()) {
@@ -163,10 +164,9 @@ void qmp_cont(Error **errp)
         bdrv_iostatus_reset(bs);
     }
     for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-        if (bdrv_key_required(bs)) {
-            error_set(errp, QERR_DEVICE_ENCRYPTED,
-                      bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+        bdrv_add_key(bs, NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
             return;
         }
     }
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 3/4] block: New bdrv_add_key(), convert monitor to use it

[Max Reitz]

On 2015-01-29 at 04:37, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block.c               | 29 +++++++++++++++++++++++++++++
>   blockdev.c            | 24 ++----------------------
>   include/block/block.h |  1 +
>   monitor.c             | 16 +++++++++++-----
>   qmp.c                 |  8 ++++----
>   5 files changed, 47 insertions(+), 31 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH v2 4/4] block: Eliminate silly QERR_ macros used for encryption keys

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.
They're used with error_set() and qerror_report(), and expand into the
first *two* arguments.  This trickiness has become pointless.  Clean
up QERR_DEVICE_ENCRYPTED and QERR_DEVICE_NOT_ENCRYPTED.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 5 +++--
 include/qapi/qmp/qerror.h | 6 ------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index aa12160..ad6fcef 100644
--- a/block.c
+++ b/block.c
@@ -3734,14 +3734,15 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
 {
     if (key) {
         if (!bdrv_is_encrypted(bs)) {
-            error_set(errp, QERR_DEVICE_NOT_ENCRYPTED,
+            error_setg(errp, "Device '%s' is not encrypted",
                       bdrv_get_device_name(bs));
         } else if (bdrv_set_key(bs, key) < 0) {
             error_set(errp, QERR_INVALID_PASSWORD);
         }
     } else {
         if (bdrv_key_required(bs)) {
-            error_set(errp, QERR_DEVICE_ENCRYPTED,
+            error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
+                      "'%s' (%s) is encrypted",
                       bdrv_get_device_name(bs),
                       bdrv_get_encrypted_filename(bs));
         }
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index becdca6..423d8a3 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -52,9 +52,6 @@ void qerror_report_err(Error *err);
 #define QERR_COMMAND_NOT_FOUND \
     ERROR_CLASS_COMMAND_NOT_FOUND, "The command %s has not been found"
 
-#define QERR_DEVICE_ENCRYPTED \
-    ERROR_CLASS_DEVICE_ENCRYPTED, "'%s' (%s) is encrypted"
-
 #define QERR_DEVICE_HAS_NO_MEDIUM \
     ERROR_CLASS_GENERIC_ERROR, "Device '%s' has no medium"
 
@@ -73,9 +70,6 @@ void qerror_report_err(Error *err);
 #define QERR_DEVICE_NOT_ACTIVE \
     ERROR_CLASS_DEVICE_NOT_ACTIVE, "No %s device has been activated"
 
-#define QERR_DEVICE_NOT_ENCRYPTED \
-    ERROR_CLASS_GENERIC_ERROR, "Device '%s' is not encrypted"
-
 #define QERR_DEVICE_NOT_FOUND \
     ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found"
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 4/4] block: Eliminate silly QERR_ macros used for encryption keys

[Max Reitz]

On 2015-01-29 at 04:37, Markus Armbruster wrote:
> The QERR_ macros are leftovers from the days of "rich" error objects.
> They're used with error_set() and qerror_report(), and expand into the
> first *two* arguments.  This trickiness has become pointless.  Clean
> up QERR_DEVICE_ENCRYPTED and QERR_DEVICE_NOT_ENCRYPTED.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block.c                   | 5 +++--
>   include/qapi/qmp/qerror.h | 6 ------
>   2 files changed, 3 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/4] block: Cleanups around error reporting

[Max Reitz]

On 2015-01-29 at 04:36, Markus Armbruster wrote:
> v2:
> * PATCH 4: Use error_setg() [Kevin]
> * R-bys retained
>
> Markus Armbruster (4):
>    blockdev: Give find_block_job() an Error ** parameter
>    blockdev: Eliminate silly QERR_BLOCK_JOB_NOT_ACTIVE macro
>    block: New bdrv_add_key(), convert monitor to use it
>    block: Eliminate silly QERR_ macros used for encryption keys
>
>   block.c                   | 30 ++++++++++++++++++++++++++++++
>   blockdev.c                | 44 +++++++++++---------------------------------
>   include/block/block.h     |  1 +
>   include/qapi/qmp/qerror.h |  9 ---------
>   monitor.c                 | 16 +++++++++++-----
>   qmp.c                     |  8 ++++----
>   6 files changed, 57 insertions(+), 51 deletions(-)

Thanks, applied to my block tree:

https://github.com/XanClic/qemu/commits/block