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

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

[Markus Armbruster]

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                   | 31 +++++++++++++++++++++++++++++++
 blockdev.c                | 44 +++++++++++---------------------------------
 include/block/block.h     |  1 +
 include/qapi/qmp/qerror.h |  9 ---------
 monitor.c                 | 16 +++++++++++-----
 qmp.c                     |  8 ++++----
 6 files changed, 58 insertions(+), 51 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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>
---
 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

[Qemu-devel] [PATCH 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>
---
 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

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

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@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 cbe4a32..ba005f3 100644
--- a/block.c
+++ b/block.c
@@ -3722,6 +3722,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 1808e41..99876dd 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 0b4f131..8c6c232 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

[Qemu-devel] [PATCH 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>
---
 block.c                   | 6 ++++--
 include/qapi/qmp/qerror.h | 6 ------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index ba005f3..4cf6d29 100644
--- a/block.c
+++ b/block.c
@@ -3737,14 +3737,16 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                      "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 0/4] block: Cleanups around error reporting

[Eric Blake]

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

On 01/14/2015 07:31 AM, Markus Armbruster wrote:
> 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

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

> 
>  block.c                   | 31 +++++++++++++++++++++++++++++++
>  blockdev.c                | 44 +++++++++++---------------------------------
>  include/block/block.h     |  1 +
>  include/qapi/qmp/qerror.h |  9 ---------
>  monitor.c                 | 16 +++++++++++-----
>  qmp.c                     |  8 ++++----
>  6 files changed, 58 insertions(+), 51 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 4/4] block: Eliminate silly QERR_ macros used for encryption keys

[Kevin Wolf]

Am 14.01.2015 um 15:31 hat Markus Armbruster geschrieben:
> 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>
> ---
>  block.c                   | 6 ++++--
>  include/qapi/qmp/qerror.h | 6 ------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ba005f3..4cf6d29 100644
> --- a/block.c
> +++ b/block.c
> @@ -3737,14 +3737,16 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> +                      "Device '%s' is not encrypted",
>                        bdrv_get_device_name(bs));

Why not error_setg()?

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.01.2015 um 15:31 hat Markus Armbruster geschrieben:
>> 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>
>> ---
>>  block.c                   | 6 ++++--
>>  include/qapi/qmp/qerror.h | 6 ------
>>  2 files changed, 4 insertions(+), 8 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index ba005f3..4cf6d29 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3737,14 +3737,16 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
>> +                      "Device '%s' is not encrypted",
>>                        bdrv_get_device_name(bs));
>
> Why not error_setg()?

Because I forgot?  I'll respin unless you prefer to fix it up on commit.

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

[Kevin Wolf]

Am 15.01.2015 um 13:40 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.01.2015 um 15:31 hat Markus Armbruster geschrieben:
> >> 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>
> >> ---
> >>  block.c                   | 6 ++++--
> >>  include/qapi/qmp/qerror.h | 6 ------
> >>  2 files changed, 4 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/block.c b/block.c
> >> index ba005f3..4cf6d29 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3737,14 +3737,16 @@ 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_set(errp, ERROR_CLASS_GENERIC_ERROR,
> >> +                      "Device '%s' is not encrypted",
> >>                        bdrv_get_device_name(bs));
> >
> > Why not error_setg()?
> 
> Because I forgot?  I'll respin unless you prefer to fix it up on commit.

I would fix it on commit, but it's Stefan's week, so whatever he prefers.

Kevin