[PATCH v2 00/13] block: deal with errp: part I

[PATCH v2 00/13] block: deal with errp: part I

[Vladimir Sementsov-Ogievskiy]

v2:
01-07: add Greg's and Alberto's r-bs
08: fix wording in commit message
    add Greg's r-b
09: fix header_updated logic, add comment, drop unrelated style-change [Alberto]
10: - fix commit header
    - add note about ERRP_GUARD in commit message
    - add Greg's r-b
11: add Greg's and Alberto's r-bs
12: was "[PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp"
    - drop wrong update of qcow2_do_open() (and ERRP_GUARD() fix
        for qcow2_do_open becomes unrelated, drop it too, will
        update later)
    - reword commit message correspondingly
13: add Alberto's r-b

"[PATCH 07/14] block/blklogwrites: drop error propagation" is dropped
for now, as it needs separate small series.

Vladimir Sementsov-Ogievskiy (13):
  block: return status from bdrv_append and friends
  block: use return status of bdrv_append()
  block: check return value of bdrv_open_child and drop error
    propagation
  blockdev: fix drive_backup_prepare() missed error
  block: drop extra error propagation for bdrv_set_backing_hd
  block/mirror: drop extra error propagation in commit_active_start()
  blockjob: return status from block_job_set_speed()
  block/qcow2: qcow2_get_specific_info(): drop error propagation
  block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
  block/qcow2-bitmap: return status from
    qcow2_store_persistent_dirty_bitmaps
  block/qcow2: read_cache_sizes: return status value
  block/qcow2: simplify qcow2_co_invalidate_cache()
  block/qed: bdrv_qed_do_open: deal with errp

 block/qcow2.h               |  9 ++---
 include/block/block.h       | 12 +++----
 include/block/blockjob.h    |  2 +-
 block.c                     | 50 +++++++++++++++-------------
 block/backup-top.c          | 20 +++++-------
 block/blkdebug.c            |  6 ++--
 block/blklogwrites.c        | 10 +++---
 block/blkreplay.c           |  6 ++--
 block/blkverify.c           | 11 +++----
 block/commit.c              |  5 +--
 block/mirror.c              | 18 ++++------
 block/qcow2-bitmap.c        | 65 +++++++++++++++++++------------------
 block/qcow2.c               | 53 ++++++++++++------------------
 block/qed.c                 | 24 +++++++++-----
 block/quorum.c              |  6 ++--
 blockdev.c                  |  7 ++--
 blockjob.c                  | 18 +++++-----
 tests/test-bdrv-graph-mod.c |  6 ++--
 18 files changed, 150 insertions(+), 178 deletions(-)

-- 
2.21.3

[PATCH v2 01/13] block: return status from bdrv_append and friends

[Vladimir Sementsov-Ogievskiy]

The recommended use of qemu error api assumes returning status together
with setting errp and avoid void functions with errp parameter. Let's
improve bdrv_append and some friends to reduce error-propagation
overhead in further patches.

Choose int return status, because bdrv_replace_node() has call to
bdrv_check_update_perm(), which reports int status, which seems correct
to propagate.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block.h | 12 ++++++------
 block.c               | 39 ++++++++++++++++++++++++---------------
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 981ab5b314..a997dbf95b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -336,10 +336,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 
 BlockDriverState *bdrv_new(void);
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-                 Error **errp);
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-                       Error **errp);
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+                Error **errp);
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                      Error **errp);
 
 int bdrv_parse_aio(const char *mode, int *flags);
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
@@ -351,8 +351,8 @@ BdrvChild *bdrv_open_child(const char *filename,
                            BdrvChildRole child_role,
                            bool allow_none, Error **errp);
 BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                         Error **errp);
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+                        Error **errp);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);
 BlockDriverState *bdrv_open(const char *filename, const char *reference,
diff --git a/block.c b/block.c
index 9538af4884..f922c6d8f4 100644
--- a/block.c
+++ b/block.c
@@ -2869,14 +2869,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
  * Sets the bs->backing link of a BDS. A new reference is created; callers
  * which don't need their own reference any more must call bdrv_unref().
  */
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                          Error **errp)
 {
+    int ret = 0;
     bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
         bdrv_inherits_from_recursive(backing_hd, bs);
 
     if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
-        return;
+        return -EPERM;
     }
 
     if (backing_hd) {
@@ -2895,15 +2896,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
 
     bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
                                     bdrv_backing_role(bs), errp);
+    if (!bs->backing) {
+        ret = -EINVAL;
+        goto out;
+    }
+
     /* If backing_hd was already part of bs's backing chain, and
      * inherits_from pointed recursively to bs then let's update it to
      * point directly to bs (else it will become NULL). */
-    if (bs->backing && update_inherits_from) {
+    if (update_inherits_from) {
         backing_hd->inherits_from = bs;
     }
 
 out:
     bdrv_refresh_limits(bs, NULL);
+
+    return ret;
 }
 
 /*
@@ -4553,8 +4561,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
     return ret;
 }
 
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-                       Error **errp)
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+                      Error **errp)
 {
     BdrvChild *c, *next;
     GSList *list = NULL, *p;
@@ -4576,6 +4584,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
             continue;
         }
         if (c->frozen) {
+            ret = -EPERM;
             error_setg(errp, "Cannot change '%s' link to '%s'",
                        c->name, from->node_name);
             goto out;
@@ -4611,6 +4620,8 @@ out:
     g_slist_free(list);
     bdrv_drained_end(from);
     bdrv_unref(from);
+
+    return ret;
 }
 
 /*
@@ -4629,20 +4640,16 @@ out:
  * parents of bs_top after bdrv_append() returns. If the caller needs to keep a
  * reference of its own, it must call bdrv_ref().
  */
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
-                 Error **errp)
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+                Error **errp)
 {
-    Error *local_err = NULL;
-
-    bdrv_set_backing_hd(bs_new, bs_top, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
+    if (ret < 0) {
         goto out;
     }
 
-    bdrv_replace_node(bs_top, bs_new, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = bdrv_replace_node(bs_top, bs_new, errp);
+    if (ret < 0) {
         bdrv_set_backing_hd(bs_new, NULL, &error_abort);
         goto out;
     }
@@ -4651,6 +4658,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
      * additional reference any more. */
 out:
     bdrv_unref(bs_new);
+
+    return ret;
 }
 
 static void bdrv_delete(BlockDriverState *bs)
-- 
2.21.3

[PATCH v2 02/13] block: use return status of bdrv_append()

[Vladimir Sementsov-Ogievskiy]

Now bdrv_append returns status and we can drop all the local_err things
around it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c                     |  5 +----
 block/backup-top.c          | 20 ++++++++------------
 block/commit.c              |  5 +----
 block/mirror.c              |  6 ++----
 blockdev.c                  |  4 +---
 tests/test-bdrv-graph-mod.c |  6 +++---
 6 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index f922c6d8f4..b4e36d6dd7 100644
--- a/block.c
+++ b/block.c
@@ -3160,7 +3160,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     int64_t total_size;
     QemuOpts *opts = NULL;
     BlockDriverState *bs_snapshot = NULL;
-    Error *local_err = NULL;
     int ret;
 
     /* if snapshot, we create a temporary backing file and open it
@@ -3207,9 +3206,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
      * order to be able to return one, we have to increase
      * bs_snapshot's refcount here */
     bdrv_ref(bs_snapshot);
-    bdrv_append(bs_snapshot, bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (bdrv_append(bs_snapshot, bs, errp) < 0) {
         bs_snapshot = NULL;
         goto out;
     }
diff --git a/block/backup-top.c b/block/backup-top.c
index fe6883cc97..eb6a34b726 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,7 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
                                          BlockCopyState **bcs,
                                          Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_GUARD();
     BDRVBackupTopState *state;
     BlockDriverState *top;
     bool appended = false;
@@ -223,9 +223,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
     bdrv_drained_begin(source);
 
     bdrv_ref(top);
-    bdrv_append(top, source, &local_err);
-    if (local_err) {
-        error_prepend(&local_err, "Cannot append backup-top filter: ");
+    if (bdrv_append(top, source, errp) < 0) {
+        error_prepend(errp, "Cannot append backup-top filter: ");
         goto fail;
     }
     appended = true;
@@ -235,18 +234,16 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
      * we want.
      */
     state->active = true;
-    bdrv_child_refresh_perms(top, top->backing, &local_err);
-    if (local_err) {
-        error_prepend(&local_err,
-                      "Cannot set permissions for backup-top filter: ");
+    if (bdrv_child_refresh_perms(top, top->backing, errp) < 0) {
+        error_prepend(errp, "Cannot set permissions for backup-top filter: ");
         goto fail;
     }
 
     state->cluster_size = cluster_size;
     state->bcs = block_copy_state_new(top->backing, state->target,
-                                      cluster_size, write_flags, &local_err);
-    if (local_err) {
-        error_prepend(&local_err, "Cannot create block-copy-state: ");
+                                      cluster_size, write_flags, errp);
+    if (!state->bcs) {
+        error_prepend(errp, "Cannot create block-copy-state: ");
         goto fail;
     }
     *bcs = state->bcs;
@@ -264,7 +261,6 @@ fail:
     }
 
     bdrv_drained_end(source);
-    error_propagate(errp, local_err);
 
     return NULL;
 }
diff --git a/block/commit.c b/block/commit.c
index 1e85c306cc..6da0902f9d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -254,7 +254,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     BlockDriverState *iter;
     BlockDriverState *commit_top_bs = NULL;
     BlockDriverState *filtered_base;
-    Error *local_err = NULL;
     int64_t base_size, top_size;
     uint64_t base_perms, iter_shared_perms;
     int ret;
@@ -312,10 +311,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
 
     commit_top_bs->total_sectors = top->total_sectors;
 
-    bdrv_append(commit_top_bs, top, &local_err);
-    if (local_err) {
+    if (bdrv_append(commit_top_bs, top, errp) < 0) {
         commit_top_bs = NULL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index 26acf4af6f..b3778248b8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1560,7 +1560,6 @@ static BlockJob *mirror_start_job(
     BlockDriverState *mirror_top_bs;
     bool target_is_backing;
     uint64_t target_perms, target_shared_perms;
-    Error *local_err = NULL;
     int ret;
 
     if (granularity == 0) {
@@ -1609,12 +1608,11 @@ static BlockJob *mirror_start_job(
      * it alive until block_job_create() succeeds even if bs has no parent. */
     bdrv_ref(mirror_top_bs);
     bdrv_drained_begin(bs);
-    bdrv_append(mirror_top_bs, bs, &local_err);
+    ret = bdrv_append(mirror_top_bs, bs, errp);
     bdrv_drained_end(bs);
 
-    if (local_err) {
+    if (ret < 0) {
         bdrv_unref(mirror_top_bs);
-        error_propagate(errp, local_err);
         return NULL;
     }
 
diff --git a/blockdev.c b/blockdev.c
index 7f2561081e..b9803e553f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1581,9 +1581,7 @@ static void external_snapshot_prepare(BlkActionState *common,
      * can fail, so we need to do it in .prepare; undoing it for abort is
      * always possible. */
     bdrv_ref(state->new_bs);
-    bdrv_append(state->new_bs, state->old_bs, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (bdrv_append(state->new_bs, state->old_bs, errp) < 0) {
         goto out;
     }
     state->overlay_appended = true;
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index 8cff13830e..2b71601c24 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
  */
 static void test_update_perm_tree(void)
 {
-    Error *local_err = NULL;
+    int ret;
 
     BlockBackend *root = blk_new(qemu_get_aio_context(),
                                  BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
@@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
     bdrv_attach_child(filter, bs, "child", &child_of_bds,
                       BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
 
-    bdrv_append(filter, bs, &local_err);
-    error_free_or_abort(&local_err);
+    ret = bdrv_append(filter, bs, NULL);
+    assert(ret < 0);
 
     blk_unref(root);
 }
-- 
2.21.3

[PATCH v2 03/13] block: check return value of bdrv_open_child and drop error propagation

[Vladimir Sementsov-Ogievskiy]

This patch is generated by cocci script:

@@
symbol bdrv_open_child, errp, local_err;
expression file;
@@

  file = bdrv_open_child(...,
-                        &local_err
+                        errp
                        );
- if (local_err)
+ if (!file)
  {
      ...
-     error_propagate(errp, local_err);
      ...
  }

with command

spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80 --use-gitgrep block

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/blkdebug.c     |  6 ++----
 block/blklogwrites.c | 10 ++++------
 block/blkreplay.c    |  6 ++----
 block/blkverify.c    | 11 ++++-------
 block/qcow2.c        |  5 ++---
 block/quorum.c       |  6 ++----
 6 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index eecbf3e5c4..5716b817ae 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -464,7 +464,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
-    Error *local_err = NULL;
     int ret;
     uint64_t align;
 
@@ -494,10 +493,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
                                bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-                               false, &local_err);
-    if (local_err) {
+                               false, errp);
+    if (!bs->file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto out;
     }
 
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 13ae63983b..b7579370a3 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
     /* Open the file */
     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false,
-                               &local_err);
-    if (local_err) {
+                               errp);
+    if (!bs->file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
     /* Open the log file */
     s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_of_bds,
-                                  BDRV_CHILD_METADATA, false, &local_err);
-    if (local_err) {
+                                  BDRV_CHILD_METADATA, false, errp);
+    if (!s->log_file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 30a0f5d57a..4a247752fd 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -23,16 +23,14 @@ typedef struct Request {
 static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
                           Error **errp)
 {
-    Error *local_err = NULL;
     int ret;
 
     /* Open the image file */
     bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-                               false, &local_err);
-    if (local_err) {
+                               false, errp);
+    if (!bs->file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 4aed53ab59..95ae73e2aa 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
-    Error *local_err = NULL;
     int ret;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
                                bs, &child_of_bds,
                                BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
-                               false, &local_err);
-    if (local_err) {
+                               false, errp);
+    if (!bs->file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
     /* Open the test file */
     s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
                                    "test", bs, &child_of_bds, BDRV_CHILD_DATA,
-                                   false, &local_err);
-    if (local_err) {
+                                   false, errp);
+    if (!s->test_file) {
         ret = -EINVAL;
-        error_propagate(errp, local_err);
         goto fail;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index b05512718c..41a29072e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1612,9 +1612,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     /* Open external data file */
     s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
                                    &child_of_bds, BDRV_CHILD_DATA,
-                                   true, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                                   true, errp);
+    if (!s->data_file) {
         ret = -EINVAL;
         goto fail;
     }
diff --git a/block/quorum.c b/block/quorum.c
index e846a7e892..2ebe0ba16d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -900,7 +900,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
                        Error **errp)
 {
     BDRVQuorumState *s = bs->opaque;
-    Error *local_err = NULL;
     QemuOpts *opts = NULL;
     const char *pattern_str;
     bool *opened;
@@ -978,9 +977,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
 
         s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
                                          &child_of_bds, BDRV_CHILD_DATA, false,
-                                         &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+                                         errp);
+        if (!s->children[i]) {
             ret = -EINVAL;
             goto close_exit;
         }
-- 
2.21.3

[PATCH v2 04/13] blockdev: fix drive_backup_prepare() missed error

[Vladimir Sementsov-Ogievskiy]

We leak local_err and don't report failure to the caller. It's
definitely wrong, let's fix.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b9803e553f..d6bde81ad4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1813,8 +1813,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     aio_context_acquire(aio_context);
 
     if (set_backing_hd) {
-        bdrv_set_backing_hd(target_bs, source, &local_err);
-        if (local_err) {
+        if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
             goto unref;
         }
     }
-- 
2.21.3

[PATCH v2 05/13] block: drop extra error propagation for bdrv_set_backing_hd

[Vladimir Sementsov-Ogievskiy]

bdrv_set_backing_hd now returns status, let's use it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index b4e36d6dd7..1cf825c349 100644
--- a/block.c
+++ b/block.c
@@ -3015,11 +3015,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
 
     /* Hook up the backing file link; drop our reference, bs owns the
      * backing_hd reference now */
-    bdrv_set_backing_hd(bs, backing_hd, &local_err);
+    ret = bdrv_set_backing_hd(bs, backing_hd, errp);
     bdrv_unref(backing_hd);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
+    if (ret < 0) {
         goto free_exit;
     }
 
-- 
2.21.3

[PATCH v2 06/13] block/mirror: drop extra error propagation in commit_active_start()

[Vladimir Sementsov-Ogievskiy]

Let's check return value of mirror_start_job to check for failure
instead of local_err.

Rename ret to job, as ret is usually integer variable.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index b3778248b8..f7c624d6a9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1851,8 +1851,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
                               bool auto_complete, Error **errp)
 {
     bool base_read_only;
-    Error *local_err = NULL;
-    BlockJob *ret;
+    BlockJob *job;
 
     base_read_only = bdrv_is_read_only(base);
 
@@ -1862,19 +1861,18 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
         }
     }
 
-    ret = mirror_start_job(
+    job = mirror_start_job(
                      job_id, bs, creation_flags, base, NULL, speed, 0, 0,
                      MIRROR_LEAVE_BACKING_CHAIN, false,
                      on_error, on_error, true, cb, opaque,
                      &commit_active_job_driver, false, base, auto_complete,
                      filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
-                     &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+                     errp);
+    if (!job) {
         goto error_restore_flags;
     }
 
-    return ret;
+    return job;
 
 error_restore_flags:
     /* ignore error and errp for bdrv_reopen, because we want to propagate
-- 
2.21.3

[PATCH v2 07/13] blockjob: return status from block_job_set_speed()

[Vladimir Sementsov-Ogievskiy]

Better to return status together with setting errp. It allows to avoid
error propagation in the caller.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 include/block/blockjob.h |  2 +-
 blockjob.c               | 18 ++++++++----------
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 35faa3aa26..d200f33c10 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
  * Set a rate-limiting parameter for the job; the actual meaning may
  * vary depending on the job type.
  */
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
 
 /**
  * block_job_query:
diff --git a/blockjob.c b/blockjob.c
index 470facfd47..afddf7a1fb 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job)
     return timer_pending(&job->sleep_timer);
 }
 
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     int64_t old_speed = job->speed;
 
-    if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) {
-        return;
+    if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
+        return false;
     }
     if (speed < 0) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
                    "a non-negative value");
-        return;
+        return false;
     }
 
     ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME);
 
     job->speed = speed;
     if (speed && speed <= old_speed) {
-        return;
+        return true;
     }
 
     /* kick only if a timer is pending */
     job_enter_cond(&job->job, job_timer_pending);
+
+    return true;
 }
 
 int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
@@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
 
     /* Only set speed when necessary to avoid NotSupported error */
     if (speed != 0) {
-        Error *local_err = NULL;
-
-        block_job_set_speed(job, speed, &local_err);
-        if (local_err) {
+        if (!block_job_set_speed(job, speed, errp)) {
             job_early_fail(&job->job);
-            error_propagate(errp, local_err);
             return NULL;
         }
     }
-- 
2.21.3

[PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Vladimir Sementsov-Ogievskiy]

Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface is rather
weird.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 block/qcow2.h        |  4 ++--
 block/qcow2-bitmap.c | 27 +++++++++++++--------------
 block/qcow2.c        | 10 +++-------
 3 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index b71e444fca..6eac088f1c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -973,8 +973,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
-                                                Error **errp);
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+                                Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d7a31a8ddc..4f6138f544 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1093,30 +1093,29 @@ static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
 /*
  * qcow2_get_bitmap_info_list()
  * Returns a list of QCOW2 bitmap details.
- * In case of no bitmaps, the function returns NULL and
- * the @errp parameter is not set.
- * When bitmap information can not be obtained, the function returns
- * NULL and the @errp parameter is set.
+ * On success return true with bm_list set (probably to NULL, if no bitmaps),
+ * on failure return false with errp set.
  */
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
-                                                Error **errp)
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+                                Qcow2BitmapInfoList **info_list, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
     Qcow2Bitmap *bm;
-    Qcow2BitmapInfoList *list = NULL;
-    Qcow2BitmapInfoList **plist = &list;
 
     if (s->nb_bitmaps == 0) {
-        return NULL;
+        *info_list = NULL;
+        return true;
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
-    if (bm_list == NULL) {
-        return NULL;
+    if (!bm_list) {
+        return false;
     }
 
+    *info_list = NULL;
+
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
         Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
         Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
@@ -1124,13 +1123,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
         info->name = g_strdup(bm->name);
         info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
         obj->value = info;
-        *plist = obj;
-        plist = &obj->next;
+        *info_list = obj;
+        info_list = &obj->next;
     }
 
     bitmap_list_free(bm_list);
 
-    return list;
+    return true;
 }
 
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
diff --git a/block/qcow2.c b/block/qcow2.c
index 41a29072e6..8c89c98978 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5038,12 +5038,10 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
     BDRVQcow2State *s = bs->opaque;
     ImageInfoSpecific *spec_info;
     QCryptoBlockInfo *encrypt_info = NULL;
-    Error *local_err = NULL;
 
     if (s->crypto != NULL) {
-        encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        encrypt_info = qcrypto_block_get_info(s->crypto, errp);
+        if (!encrypt_info) {
             return NULL;
         }
     }
@@ -5060,9 +5058,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
         };
     } else if (s->qcow_version == 3) {
         Qcow2BitmapInfoList *bitmaps;
-        bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) {
             qapi_free_ImageInfoSpecific(spec_info);
             qapi_free_QCryptoBlockInfo(encrypt_info);
             return NULL;
-- 
2.21.3

[PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Vladimir Sementsov-Ogievskiy]

It's recommended for bool functions with errp to return true on success
and false on failure. Non-standard interfaces don't help to understand
the code. The change is also needed to reduce error propagation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h        |  3 ++-
 block/qcow2-bitmap.c | 25 ++++++++++++++-----------
 block/qcow2.c        |  6 ++----
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 6eac088f1c..3c64dcda33 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
 int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+                              Error **errp);
 bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                 Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 4f6138f544..500175f4e8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -962,25 +962,26 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
     bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
 }
 
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
+/*
+ * Return true on success, false on failure. Anyway, if header_updated
+ * provided set it appropriately.
  */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+                              Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
     Qcow2Bitmap *bm;
     GSList *created_dirty_bitmaps = NULL;
-    bool header_updated = false;
     bool needs_update = false;
 
+    if (header_updated) {
+        *header_updated = false;
+    }
+
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
-        return false;
+        return true;
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
@@ -1036,7 +1037,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
             error_setg_errno(errp, -ret, "Can't update bitmap directory");
             goto fail;
         }
-        header_updated = true;
+        if (header_updated) {
+            *header_updated = true;
+        }
     }
 
     if (!can_write(bs)) {
@@ -1047,7 +1050,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
     g_slist_free(created_dirty_bitmaps);
     bitmap_list_free(bm_list);
 
-    return header_updated;
+    return true;
 
 fail:
     g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c89c98978..c4b86df7c0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     unsigned int len, i;
     int ret = 0;
     QCowHeader header;
-    Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
     bool update_header = false;
@@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
 
     if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
         /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
-        bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
-        if (local_err != NULL) {
-            error_propagate(errp, local_err);
+        bool header_updated;
+        if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
             ret = -EINVAL;
             goto fail;
         }
-- 
2.21.3

[PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

It's better to return status together with setting errp. It makes
possible to avoid error propagation.

While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
above ERRP_GUARD() definition in include/qapi/error.h)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
 block/qcow2.h        |  2 +-
 block/qcow2-bitmap.c | 13 ++++++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 3c64dcda33..7884a5088d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -978,7 +978,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
                                 Qcow2BitmapInfoList **info_list, Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
                                           bool release_stored, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
 bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 500175f4e8..b8ff347885 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1534,9 +1534,10 @@ out:
  * readonly to begin with, and whether we opened directly or reopened to that
  * state shouldn't matter for the state we get afterward.
  */
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
                                           bool release_stored, Error **errp)
 {
+    ERRP_GUARD();
     BdrvDirtyBitmap *bitmap;
     BDRVQcow2State *s = bs->opaque;
     uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1556,7 +1557,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
         bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                    s->bitmap_directory_size, errp);
         if (bm_list == NULL) {
-            return;
+            return false;
         }
     }
 
@@ -1671,7 +1672,7 @@ success:
     }
 
     bitmap_list_free(bm_list);
-    return;
+    return true;
 
 fail:
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1689,16 +1690,14 @@ fail:
     }
 
     bitmap_list_free(bm_list);
+    return false;
 }
 
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
 {
     BdrvDirtyBitmap *bitmap;
-    Error *local_err = NULL;
 
-    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
-    if (local_err != NULL) {
-        error_propagate(errp, local_err);
+    if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
         return -EINVAL;
     }
 
-- 
2.21.3

[PATCH v2 11/13] block/qcow2: read_cache_sizes: return status value

[Vladimir Sementsov-Ogievskiy]

It's better to return status together with setting errp. It allows to
reduce error propagation.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c4b86df7c0..2b6ec4b757 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs,
     cache_clean_timer_init(bs, new_context);
 }
 
-static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
+static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
                              uint64_t *l2_cache_size,
                              uint64_t *l2_cache_entry_size,
                              uint64_t *refcount_cache_size, Error **errp)
@@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
             error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
                        " and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
                        "at the same time");
-            return;
+            return false;
         } else if (l2_cache_size_set &&
                    (l2_cache_max_setting > combined_cache_size)) {
             error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
                        QCOW2_OPT_CACHE_SIZE);
-            return;
+            return false;
         } else if (*refcount_cache_size > combined_cache_size) {
             error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
                        QCOW2_OPT_CACHE_SIZE);
-            return;
+            return false;
         }
 
         if (l2_cache_size_set) {
@@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
         error_setg(errp, "L2 cache entry size must be a power of two "
                    "between %d and the cluster size (%d)",
                    1 << MIN_CLUSTER_BITS, s->cluster_size);
-        return;
+        return false;
     }
+
+    return true;
 }
 
 typedef struct Qcow2ReopenState {
@@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     int i;
     const char *encryptfmt;
     QDict *encryptopts = NULL;
-    Error *local_err = NULL;
     int ret;
 
     qdict_extract_subqdict(options, &encryptopts, "encrypt.");
@@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
     }
 
     /* get L2 table/refcount block cache size from command line options */
-    read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
-                     &refcount_cache_size, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
+                          &refcount_cache_size, errp)) {
         ret = -EINVAL;
         goto fail;
     }
-- 
2.21.3

[PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Vladimir Sementsov-Ogievskiy]

qcow2_do_open correctly sets errp on each failure path. So, we can
simplify code in qcow2_co_invalidate_cache() and drop explicit error
propagation. We should use ERRP_GUARD() (accordingly to comment in
include/qapi/error.h) together with error_append() call which we add to
avoid problems with error_fatal.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2b6ec4b757..cd5f48d3fb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
 static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
                                                    Error **errp)
 {
+    ERRP_GUARD();
     BDRVQcow2State *s = bs->opaque;
     int flags = s->flags;
     QCryptoBlock *crypto = NULL;
     QDict *options;
-    Error *local_err = NULL;
     int ret;
 
     /*
@@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
 
     flags &= ~BDRV_O_INACTIVE;
     qemu_co_mutex_lock(&s->lock);
-    ret = qcow2_do_open(bs, options, flags, &local_err);
+    ret = qcow2_do_open(bs, options, flags, errp);
     qemu_co_mutex_unlock(&s->lock);
     qobject_unref(options);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "Could not reopen qcow2 layer: ");
-        bs->drv = NULL;
-        return;
-    } else if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
+    if (ret < 0) {
+        error_prepend(errp, "Could not reopen qcow2 layer: ");
         bs->drv = NULL;
         return;
     }
-- 
2.21.3

[PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp

[Vladimir Sementsov-Ogievskiy]

Set errp always on failure. Generic bdrv_open_driver supports driver
functions which can return negative value and forget to set errp.
That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
this way. This allows to simplify code in
bdrv_qed_co_invalidate_cache().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qed.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index b27e7546ca..f45c640513 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
     if (ret < 0) {
+        error_setg(errp, "Failed to read QED header");
         return ret;
     }
     qed_header_le_to_cpu(&le_header, &s->header);
@@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
         return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
+        error_setg(errp, "QED cluster size is invalid");
         return -EINVAL;
     }
 
     /* Round down file size to the last cluster */
     file_size = bdrv_getlength(bs->file->bs);
     if (file_size < 0) {
+        error_setg(errp, "Failed to get file length");
         return file_size;
     }
     s->file_size = qed_start_of_cluster(s, file_size);
 
     if (!qed_is_table_size_valid(s->header.table_size)) {
+        error_setg(errp, "QED table size is invalid");
         return -EINVAL;
     }
     if (!qed_is_image_size_valid(s->header.image_size,
                                  s->header.cluster_size,
                                  s->header.table_size)) {
+        error_setg(errp, "QED image size is invalid");
         return -EINVAL;
     }
     if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        error_setg(errp, "QED table offset is invalid");
         return -EINVAL;
     }
 
@@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     /* Header size calculation must not overflow uint32_t */
     if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
+        error_setg(errp, "QED header size is too large");
         return -EINVAL;
     }
 
@@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
         if ((uint64_t)s->header.backing_filename_offset +
             s->header.backing_filename_size >
             s->header.cluster_size * s->header.header_size) {
+            error_setg(errp, "QED backing filename offset is invalid");
             return -EINVAL;
         }
 
@@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
                               bs->auto_backing_file,
                               sizeof(bs->auto_backing_file));
         if (ret < 0) {
+            error_setg(errp, "Failed to read backing filename");
             return ret;
         }
         pstrcpy(bs->backing_file, sizeof(bs->backing_file),
@@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
         ret = qed_write_header_sync(s);
         if (ret) {
+            error_setg(errp, "Failed to update header");
             return ret;
         }
 
@@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     ret = qed_read_l1_table_sync(s);
     if (ret) {
+        error_setg(errp, "Failed to read L1 table");
         goto out;
     }
 
@@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
             ret = qed_check(s, &result, true);
             if (ret) {
+                error_setg(errp, "Image corrupted");
                 goto out;
             }
         }
@@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
                                                       Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
-    Error *local_err = NULL;
     int ret;
 
     bdrv_qed_close(bs);
 
     bdrv_qed_init_state(bs);
     qemu_co_mutex_lock(&s->table_lock);
-    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
+    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
     qemu_co_mutex_unlock(&s->table_lock);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "Could not reopen qed layer: ");
-        return;
-    } else if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not reopen qed layer");
-        return;
+    if (ret < 0) {
+        error_prepend(errp, "Could not reopen qed layer: ");
     }
 }
 
-- 
2.21.3

Re: [PATCH v2 00/13] block: deal with errp: part I

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200917195519.19589-1-vsementsov@virtuozzo.com/



Hi,

This series failed build test on FreeBSD host. Please find the details below.






The full log is available at
http://patchew.org/logs/20200917195519.19589-1-vsementsov@virtuozzo.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 00/13] block: deal with errp: part I

[Vladimir Sementsov-Ogievskiy]

17.09.2020 23:15, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200917195519.19589-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series failed build test on FreeBSD host. Please find the details below.
> 
> 
> 
> 
> 
> 
> The full log is available at
> http://patchew.org/logs/20200917195519.19589-1-vsementsov@virtuozzo.com/testing.FreeBSD/?type=message.

Link is broken, it shows: "N/A. Internal error while reading log file"


-- 
Best regards,
Vladimir

Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Alberto Garcia]

On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's recommended for bool functions with errp to return true on success
> and false on failure. Non-standard interfaces don't help to understand
> the code. The change is also needed to reduce error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +/*
> + * Return true on success, false on failure. Anyway, if header_updated
> + * provided set it appropriately.
>   */

I'm not a native speaker but it sounds a bit odd to me. Maybe "If
header_updated is not NULL then it is set appropriately regardless of
the return value".

But I'm fine with your version, so

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Vladimir Sementsov-Ogievskiy]

18.09.2020 17:54, Alberto Garcia wrote:
> On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> It's recommended for bool functions with errp to return true on success
>> and false on failure. Non-standard interfaces don't help to understand
>> the code. The change is also needed to reduce error propagation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>> +/*
>> + * Return true on success, false on failure. Anyway, if header_updated
>> + * provided set it appropriately.
>>    */
> 
> I'm not a native speaker but it sounds a bit odd to me. Maybe "If
> header_updated is not NULL then it is set appropriately regardless of
> the return value".

That's better I think, thanks.

> 
> But I'm fine with your version, so
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Berto
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Alberto Garcia]

On Thu 17 Sep 2020 09:55:14 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> Don't use error propagation in qcow2_get_specific_info(). For this
> refactor qcow2_get_bitmap_info_list, its current interface is rather
> weird.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>

> - * In case of no bitmaps, the function returns NULL and
> - * the @errp parameter is not set.
> - * When bitmap information can not be obtained, the function returns
> - * NULL and the @errp parameter is set.
> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
> + * on failure return false with errp set.

I still find the 'probably' thing odd :-) I think the API documentation
should describe what the function does and under which conditions, not
the probability of the outcomes.

>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>          Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
>          Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
> @@ -1124,13 +1123,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>          info->name = g_strdup(bm->name);
>          info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
>          obj->value = info;
> -        *plist = obj;
> -        plist = &obj->next;
> +        *info_list = obj;
> +        info_list = &obj->next;
>      }

You were right with this, I got confused by the fact that you are
modifying the pointer provided by the user.

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Greg Kurz]

On Thu, 17 Sep 2020 22:55:15 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's recommended for bool functions with errp to return true on success
> and false on failure. Non-standard interfaces don't help to understand
> the code. The change is also needed to reduce error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

With the documentation change suggested by Berto,

Reviewed-by: Greg Kurz <groug@kaod.org>

>  block/qcow2.h        |  3 ++-
>  block/qcow2-bitmap.c | 25 ++++++++++++++-----------
>  block/qcow2.c        |  6 ++----
>  3 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6eac088f1c..3c64dcda33 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
>  int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>                                    void **refcount_table,
>                                    int64_t *refcount_table_size);
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> +                              Error **errp);
>  bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
>                                  Qcow2BitmapInfoList **info_list, Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 4f6138f544..500175f4e8 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -962,25 +962,26 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
>      bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
>  }
>  
> -/* qcow2_load_dirty_bitmaps()
> - * Return value is a hint for caller: true means that the Qcow2 header was
> - * updated. (false doesn't mean that the header should be updated by the
> - * caller, it just means that updating was not needed or the image cannot be
> - * written to).
> - * On failure the function returns false.
> +/*
> + * Return true on success, false on failure. Anyway, if header_updated
> + * provided set it appropriately.
>   */
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> +                              Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      Qcow2BitmapList *bm_list;
>      Qcow2Bitmap *bm;
>      GSList *created_dirty_bitmaps = NULL;
> -    bool header_updated = false;
>      bool needs_update = false;
>  
> +    if (header_updated) {
> +        *header_updated = false;
> +    }
> +
>      if (s->nb_bitmaps == 0) {
>          /* No bitmaps - nothing to do */
> -        return false;
> +        return true;
>      }
>  
>      bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> @@ -1036,7 +1037,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              error_setg_errno(errp, -ret, "Can't update bitmap directory");
>              goto fail;
>          }
> -        header_updated = true;
> +        if (header_updated) {
> +            *header_updated = true;
> +        }
>      }
>  
>      if (!can_write(bs)) {
> @@ -1047,7 +1050,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      g_slist_free(created_dirty_bitmaps);
>      bitmap_list_free(bm_list);
>  
> -    return header_updated;
> +    return true;
>  
>  fail:
>      g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8c89c98978..c4b86df7c0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>      unsigned int len, i;
>      int ret = 0;
>      QCowHeader header;
> -    Error *local_err = NULL;
>      uint64_t ext_end;
>      uint64_t l1_vm_state_index;
>      bool update_header = false;
> @@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>  
>      if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
>          /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
> -        bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
> -        if (local_err != NULL) {
> -            error_propagate(errp, local_err);
> +        bool header_updated;
> +        if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
>              ret = -EINVAL;
>              goto fail;
>          }

Re: [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps

[Alberto Garcia]

On Thu 17 Sep 2020 09:55:16 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
> usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
> above ERRP_GUARD() definition in include/qapi/error.h)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Alberto Garcia]

On Thu 17 Sep 2020 09:55:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation. We should use ERRP_GUARD() (accordingly to comment in
> include/qapi/error.h) together with error_append() call which we add to
> avoid problems with error_fatal.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Greg Kurz]

On Thu, 17 Sep 2020 22:55:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation. We should use ERRP_GUARD() (accordingly to comment in
> include/qapi/error.h) together with error_append() call which we add to
> avoid problems with error_fatal.
> 

The wording gives the impression that we add error_append() to avoid problems
with error_fatal which is certainly not true. Also it isn't _append() but
_prepend() :)

What about ?

"Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
 to avoid problems with the error_prepend() call if errp is &error_fatal."

With that fixed,

Reviewed-by: Greg Kurz <groug@kaod.org>

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2b6ec4b757..cd5f48d3fb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
>  static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>                                                     Error **errp)
>  {
> +    ERRP_GUARD();
>      BDRVQcow2State *s = bs->opaque;
>      int flags = s->flags;
>      QCryptoBlock *crypto = NULL;
>      QDict *options;
> -    Error *local_err = NULL;
>      int ret;
>  
>      /*
> @@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>  
>      flags &= ~BDRV_O_INACTIVE;
>      qemu_co_mutex_lock(&s->lock);
> -    ret = qcow2_do_open(bs, options, flags, &local_err);
> +    ret = qcow2_do_open(bs, options, flags, errp);
>      qemu_co_mutex_unlock(&s->lock);
>      qobject_unref(options);
> -    if (local_err) {
> -        error_propagate_prepend(errp, local_err,
> -                                "Could not reopen qcow2 layer: ");
> -        bs->drv = NULL;
> -        return;
> -    } else if (ret < 0) {
> -        error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
> +    if (ret < 0) {
> +        error_prepend(errp, "Could not reopen qcow2 layer: ");
>          bs->drv = NULL;
>          return;
>      }

Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Alberto Garcia]

On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
>> qcow2_do_open correctly sets errp on each failure path. So, we can
>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
>> propagation. We should use ERRP_GUARD() (accordingly to comment in
>> include/qapi/error.h) together with error_append() call which we add to
>> avoid problems with error_fatal.
>> 
>
> The wording gives the impression that we add error_append() to avoid problems
> with error_fatal which is certainly not true. Also it isn't _append() but
> _prepend() :)
>
> What about ?
>
> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
>  to avoid problems with the error_prepend() call if errp is
>  &error_fatal."

I had to go to the individual error functions to see what "it doesn't
work with &error_fatal" actually means.

So in a case like qcow2_do_open() which has:

   error_setg(errp, ...)
   error_append_hint(errp, ...)

As far as I can see this works just fine without ERRP_GUARD() and with
error_fatal, the difference is that if we don't use the guard then the
process exists during error_setg(), and if we use the guard it exists
during the implicit error_propagate() call triggered by its destruction
at the end of the function. In this latter case the printed error
message would include the hint.

Berto

Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Vladimir Sementsov-Ogievskiy]

18.09.2020 18:51, Alberto Garcia wrote:
> On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
>>> qcow2_do_open correctly sets errp on each failure path. So, we can
>>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
>>> propagation. We should use ERRP_GUARD() (accordingly to comment in
>>> include/qapi/error.h) together with error_append() call which we add to
>>> avoid problems with error_fatal.
>>>
>>
>> The wording gives the impression that we add error_append() to avoid problems
>> with error_fatal which is certainly not true. Also it isn't _append() but
>> _prepend() :)
>>
>> What about ?
>>
>> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
>>   to avoid problems with the error_prepend() call if errp is
>>   &error_fatal."

OK for me.

> 
> I had to go to the individual error functions to see what "it doesn't
> work with &error_fatal" actually means.
> 
> So in a case like qcow2_do_open() which has:
> 
>     error_setg(errp, ...)
>     error_append_hint(errp, ...)
> 
> As far as I can see this works just fine without ERRP_GUARD() and with
> error_fatal, the difference is that if we don't use the guard then the
> process exists during error_setg(), and if we use the guard it exists
> during the implicit error_propagate() call triggered by its destruction
> at the end of the function. In this latter case the printed error
> message would include the hint.
> 

Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal.

-- 
Best regards,
Vladimir

Re: [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp

[Greg Kurz]

On Thu, 17 Sep 2020 22:55:19 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Set errp always on failure. Generic bdrv_open_driver supports driver
> functions which can return negative value and forget to set errp.
> That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
> this way. This allows to simplify code in
> bdrv_qed_co_invalidate_cache().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  block/qed.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index b27e7546ca..f45c640513 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
>      if (ret < 0) {
> +        error_setg(errp, "Failed to read QED header");
>          return ret;
>      }
>      qed_header_le_to_cpu(&le_header, &s->header);
> @@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>          return -ENOTSUP;
>      }
>      if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
> +        error_setg(errp, "QED cluster size is invalid");
>          return -EINVAL;
>      }
>  
>      /* Round down file size to the last cluster */
>      file_size = bdrv_getlength(bs->file->bs);
>      if (file_size < 0) {
> +        error_setg(errp, "Failed to get file length");
>          return file_size;
>      }
>      s->file_size = qed_start_of_cluster(s, file_size);
>  
>      if (!qed_is_table_size_valid(s->header.table_size)) {
> +        error_setg(errp, "QED table size is invalid");
>          return -EINVAL;
>      }
>      if (!qed_is_image_size_valid(s->header.image_size,
>                                   s->header.cluster_size,
>                                   s->header.table_size)) {
> +        error_setg(errp, "QED image size is invalid");
>          return -EINVAL;
>      }
>      if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
> +        error_setg(errp, "QED table offset is invalid");
>          return -EINVAL;
>      }
>  
> @@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      /* Header size calculation must not overflow uint32_t */
>      if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
> +        error_setg(errp, "QED header size is too large");
>          return -EINVAL;
>      }
>  
> @@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>          if ((uint64_t)s->header.backing_filename_offset +
>              s->header.backing_filename_size >
>              s->header.cluster_size * s->header.header_size) {
> +            error_setg(errp, "QED backing filename offset is invalid");
>              return -EINVAL;
>          }
>  
> @@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>                                bs->auto_backing_file,
>                                sizeof(bs->auto_backing_file));
>          if (ret < 0) {
> +            error_setg(errp, "Failed to read backing filename");
>              return ret;
>          }
>          pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> @@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>          ret = qed_write_header_sync(s);
>          if (ret) {
> +            error_setg(errp, "Failed to update header");
>              return ret;
>          }
>  
> @@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      ret = qed_read_l1_table_sync(s);
>      if (ret) {
> +        error_setg(errp, "Failed to read L1 table");
>          goto out;
>      }
>  
> @@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>              ret = qed_check(s, &result, true);
>              if (ret) {
> +                error_setg(errp, "Image corrupted");
>                  goto out;
>              }
>          }
> @@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
>                                                        Error **errp)
>  {
>      BDRVQEDState *s = bs->opaque;
> -    Error *local_err = NULL;
>      int ret;
>  
>      bdrv_qed_close(bs);
>  
>      bdrv_qed_init_state(bs);
>      qemu_co_mutex_lock(&s->table_lock);
> -    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
> +    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
>      qemu_co_mutex_unlock(&s->table_lock);
> -    if (local_err) {
> -        error_propagate_prepend(errp, local_err,
> -                                "Could not reopen qed layer: ");
> -        return;
> -    } else if (ret < 0) {
> -        error_setg_errno(errp, -ret, "Could not reopen qed layer");
> -        return;
> +    if (ret < 0) {
> +        error_prepend(errp, "Could not reopen qed layer: ");
>      }
>  }
>  

Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()

[Greg Kurz]

On Fri, 18 Sep 2020 19:01:34 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 18.09.2020 18:51, Alberto Garcia wrote:
> > On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
> >>> qcow2_do_open correctly sets errp on each failure path. So, we can
> >>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> >>> propagation. We should use ERRP_GUARD() (accordingly to comment in
> >>> include/qapi/error.h) together with error_append() call which we add to
> >>> avoid problems with error_fatal.
> >>>
> >>
> >> The wording gives the impression that we add error_append() to avoid problems
> >> with error_fatal which is certainly not true. Also it isn't _append() but
> >> _prepend() :)
> >>
> >> What about ?
> >>
> >> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
> >>   to avoid problems with the error_prepend() call if errp is
> >>   &error_fatal."
> 
> OK for me.
> 
> > 
> > I had to go to the individual error functions to see what "it doesn't
> > work with &error_fatal" actually means.
> > 
> > So in a case like qcow2_do_open() which has:
> > 
> >     error_setg(errp, ...)
> >     error_append_hint(errp, ...)
> > 
> > As far as I can see this works just fine without ERRP_GUARD() and with
> > error_fatal, the difference is that if we don't use the guard then the
> > process exists during error_setg(), and if we use the guard it exists
> > during the implicit error_propagate() call triggered by its destruction
> > at the end of the function. In this latter case the printed error
> > message would include the hint.
> > 
> 
> Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal.
> 

Yeah, so rather:

"Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
 so that error_prepend() is actually called even if errp is &error_fatal."

Cheers,

--
Greg