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

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

[Vladimir Sementsov-Ogievskiy]

Hi all!

I've start to apply scripts/coccinelle/errp-guard.cocci to block
subsystem, but it turned out that in most cases error propagation can be 
simply avoided. So, here are some improvements to block layer error
reporting to avoid error propagation. It's not complete, so its called
part I, I don't want to create huge series.

Vladimir Sementsov-Ogievskiy (14):
  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()
  block/blklogwrites: drop error propagation
  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 startus from
    qcow2_store_persistent_dirty_bitmaps
  block/qcow2: read_cache_sizes: return status value
  block/qcow2: qcow2_do_open: deal with errp
  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        | 33 +++++++++-----------
 block/blkreplay.c           |  6 ++--
 block/blkverify.c           | 11 +++----
 block/commit.c              |  5 +--
 block/mirror.c              | 18 +++++------
 block/qcow2-bitmap.c        | 62 +++++++++++++++++--------------------
 block/qcow2.c               | 56 ++++++++++++++-------------------
 block/qed.c                 | 24 ++++++++------
 block/quorum.c              |  6 ++--
 blockdev.c                  |  7 ++---
 blockjob.c                  | 18 +++++------
 tests/test-bdrv-graph-mod.c |  6 ++--
 18 files changed, 159 insertions(+), 192 deletions(-)

-- 
2.21.3

[PATCH 01/14] 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>
---
 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 6e36154061..03b3cee8f8 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 2ba76b2c36..6d35449027 100644
--- a/block.c
+++ b/block.c
@@ -2866,14 +2866,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
  * Sets the backing file 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, backing_bs(bs), errp)) {
-        return;
+        return -EPERM;
     }
 
     if (backing_hd) {
@@ -2891,15 +2892,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;
 }
 
 /*
@@ -4517,8 +4525,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;
@@ -4540,6 +4548,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;
@@ -4575,6 +4584,8 @@ out:
     g_slist_free(list);
     bdrv_drained_end(from);
     bdrv_unref(from);
+
+    return ret;
 }
 
 /*
@@ -4593,20 +4604,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;
     }
@@ -4615,6 +4622,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 02/14] 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>
---
 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 6d35449027..9b624b2535 100644
--- a/block.c
+++ b/block.c
@@ -3156,7 +3156,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
@@ -3203,9 +3202,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 af2f20f346..de9d5e1634 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -192,7 +192,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;
@@ -225,9 +225,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;
@@ -237,18 +236,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;
@@ -266,7 +263,6 @@ fail:
     }
 
     bdrv_drained_end(source);
-    error_propagate(errp, local_err);
 
     return NULL;
 }
diff --git a/block/commit.c b/block/commit.c
index 7732d02dfe..7720d4729b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     CommitBlockJob *s;
     BlockDriverState *iter;
     BlockDriverState *commit_top_bs = NULL;
-    Error *local_err = NULL;
     int ret;
 
     assert(top != bs);
@@ -292,10 +291,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 e8e8844afc..ca250f765d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job(
     BlockDriverState *mirror_top_bs;
     bool target_graph_mod;
     bool target_is_backing;
-    Error *local_err = NULL;
     int ret;
 
     if (granularity == 0) {
@@ -1606,12 +1605,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 3848a9c8ab..36bef6b188 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1576,9 +1576,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 03/14] 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>
---
 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 9c08d8a005..61aaee9879 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 57315f56b4..7ef046cee9 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 da56b1a4df..10175fa399 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1613,9 +1613,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 6df9449fc2..672b09e13d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -898,7 +898,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;
@@ -976,9 +975,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 04/14] 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>
---
 blockdev.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 36bef6b188..74259527c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1797,8 +1797,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 05/14] 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>
---
 block.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 9b624b2535..c5e3a1927e 100644
--- a/block.c
+++ b/block.c
@@ -3011,11 +3011,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 06/14] 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>
---
 block/mirror.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index ca250f765d..529ba12b2a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1788,8 +1788,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);
 
@@ -1799,19 +1798,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 07/14] block/blklogwrites: drop error propagation

[Vladimir Sementsov-Ogievskiy]

It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/blklogwrites.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
         sector_size < (1ull << 24);
 }
 
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
-                                                   uint32_t sector_size,
-                                                   uint64_t nr_entries,
-                                                   Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
+                                                  uint32_t sector_size,
+                                                  uint64_t nr_entries,
+                                                  Error **errp)
 {
     uint64_t cur_sector = 1;
     uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
         if (read_ret < 0) {
             error_setg_errno(errp, -read_ret,
                              "Failed to read log entry %"PRIu64, cur_idx);
-            return (uint64_t)-1ull;
+            return read_ret;
         }
 
         if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
             error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
                        le64_to_cpu(cur_entry.flags), cur_idx);
-            return (uint64_t)-1ull;
+            return -EINVAL;
         }
 
         /* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
 {
     BDRVBlkLogWritesState *s = bs->opaque;
     QemuOpts *opts;
-    Error *local_err = NULL;
     int ret;
     uint64_t log_sector_size;
     bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
         s->nr_entries = 0;
 
         if (blk_log_writes_sector_size_valid(log_sector_size)) {
-            s->cur_log_sector =
+            int64_t cur_log_sector =
                 blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
-                                    le64_to_cpu(log_sb.nr_entries), &local_err);
-            if (local_err) {
-                ret = -EINVAL;
-                error_propagate(errp, local_err);
+                                    le64_to_cpu(log_sb.nr_entries), errp);
+            if (cur_log_sector < 0) {
+                ret = cur_log_sector;
                 goto fail_log;
             }
 
+            s->cur_log_sector = cur_log_sector;
             s->nr_entries = le64_to_cpu(log_sb.nr_entries);
         }
     } else {
-- 
2.21.3

[PATCH 08/14] 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>
---
 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 09/14] 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 rather
weird.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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 065ec3df0b..ac6a2d3e2a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -967,8 +967,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 8c34b2aef7..9b14c0791f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1090,30 +1090,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);
@@ -1121,13 +1120,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 10175fa399..eb7c82120c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5056,12 +5056,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;
         }
     }
@@ -5078,9 +5076,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 10/14] 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 | 22 +++++++++-------------
 block/qcow2.c        |  6 ++----
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ac6a2d3e2a..e7e662533b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -966,7 +966,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 9b14c0791f..f58923fce3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -959,30 +959,24 @@ 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.
- */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/* Return true on success, false on failure. */
+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 (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
-        return false;
+        return true;
     }
 
     bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
                                s->bitmap_directory_size, errp);
-    if (bm_list == NULL) {
+    if (!bm_list) {
         return false;
     }
 
@@ -1033,7 +1027,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)) {
@@ -1044,7 +1040,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 eb7c82120c..c2cd9434cc 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;
@@ -1786,9 +1785,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 11/14] block/qcow2-bitmap: return startus 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.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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 e7e662533b..49824be5c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1524,9 +1524,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;
@@ -1546,7 +1547,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;
         }
     }
 
@@ -1661,7 +1662,7 @@ success:
     }
 
     bitmap_list_free(bm_list);
-    return;
+    return true;
 
 fail:
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1679,16 +1680,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 12/14] 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>
---
 block/qcow2.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c2cd9434cc..31dd28d19e 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 13/14] block/qcow2: qcow2_do_open: deal with errp

[Vladimir Sementsov-Ogievskiy]

1. Drop extra error propagation

2. 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 qcow2_do_open to not behave this
way. This allows to simplify code in qcow2_co_invalidate_cache().

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 31dd28d19e..cc4e7dd461 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
 static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                                       int flags, Error **errp)
 {
+    ERRP_GUARD();
     BDRVQcow2State *s = bs->opaque;
     unsigned int len, i;
     int ret = 0;
@@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         report_unsupported_feature(errp, feature_table,
                                    s->incompatible_features &
                                    ~QCOW2_INCOMPAT_MASK);
+        error_setg(errp,
+                   "qcow2 header contains unknown incompatible_feature bits");
         ret = -ENOTSUP;
         g_free(feature_table);
         goto fail;
@@ -2709,11 +2712,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;
 
     /*
@@ -2731,16 +2734,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 14/14] 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>
---
 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 01/14] block: return status from bdrv_append and friends

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:17 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  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 6e36154061..03b3cee8f8 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 2ba76b2c36..6d35449027 100644
> --- a/block.c
> +++ b/block.c
> @@ -2866,14 +2866,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
>   * Sets the backing file 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, backing_bs(bs), errp)) {
> -        return;
> +        return -EPERM;
>      }
>  
>      if (backing_hd) {
> @@ -2891,15 +2892,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;
>  }
>  
>  /*
> @@ -4517,8 +4525,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;
> @@ -4540,6 +4548,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;
> @@ -4575,6 +4584,8 @@ out:
>      g_slist_free(list);
>      bdrv_drained_end(from);
>      bdrv_unref(from);
> +
> +    return ret;
>  }
>  
>  /*
> @@ -4593,20 +4604,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;
>      }
> @@ -4615,6 +4622,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)

Re: [PATCH 02/14] block: use return status of bdrv_append()

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

Just one suggestion for a follow-up below...

>  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 6d35449027..9b624b2535 100644
> --- a/block.c
> +++ b/block.c
> @@ -3156,7 +3156,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
> @@ -3203,9 +3202,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 af2f20f346..de9d5e1634 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -192,7 +192,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;
> @@ -225,9 +225,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;
> @@ -237,18 +236,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;
> @@ -266,7 +263,6 @@ fail:
>      }
>  
>      bdrv_drained_end(source);
> -    error_propagate(errp, local_err);
>  
>      return NULL;
>  }
> diff --git a/block/commit.c b/block/commit.c
> index 7732d02dfe..7720d4729b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>      CommitBlockJob *s;
>      BlockDriverState *iter;
>      BlockDriverState *commit_top_bs = NULL;
> -    Error *local_err = NULL;
>      int ret;
>  

... this is unrelated but while reviewing I've noticed that the ret
variable isn't really needed.

>      assert(top != bs);
> @@ -292,10 +291,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 e8e8844afc..ca250f765d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job(
>      BlockDriverState *mirror_top_bs;
>      bool target_graph_mod;
>      bool target_is_backing;
> -    Error *local_err = NULL;
>      int ret;
>  
>      if (granularity == 0) {
> @@ -1606,12 +1605,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 3848a9c8ab..36bef6b188 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1576,9 +1576,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);
>  }

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

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:19 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  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 9c08d8a005..61aaee9879 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 57315f56b4..7ef046cee9 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 da56b1a4df..10175fa399 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1613,9 +1613,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 6df9449fc2..672b09e13d 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -898,7 +898,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;
> @@ -976,9 +975,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;
>          }

Re: [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:20 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  blockdev.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 36bef6b188..74259527c1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1797,8 +1797,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;
>          }
>      }

Re: [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:21 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  block.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 9b624b2535..c5e3a1927e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3011,11 +3011,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;
>      }
>  

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

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:22 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  block/mirror.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index ca250f765d..529ba12b2a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1788,8 +1788,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);
>  
> @@ -1799,19 +1798,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

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:23 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's simple to avoid error propagation in blk_log_writes_open(), we
> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/blklogwrites.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 7ef046cee9..c7da507b2d 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>          sector_size < (1ull << 24);
>  }
>  
> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> -                                                   uint32_t sector_size,
> -                                                   uint64_t nr_entries,
> -                                                   Error **errp)
> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> +                                                  uint32_t sector_size,
> +                                                  uint64_t nr_entries,
> +                                                  Error **errp)
>  {
>      uint64_t cur_sector = 1;
>      uint64_t cur_idx = 0;
> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>          if (read_ret < 0) {
>              error_setg_errno(errp, -read_ret,
>                               "Failed to read log entry %"PRIu64, cur_idx);
> -            return (uint64_t)-1ull;
> +            return read_ret;

This changes the error status of blk_log_writes_open() from -EINVAL to
whatever is returned by bdrv_pread(). I guess this is an improvement
worth to be mentioned in the changelog.

>          }
>  
>          if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>              error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>                         le64_to_cpu(cur_entry.flags), cur_idx);
> -            return (uint64_t)-1ull;
> +            return -EINVAL;
>          }
>  
>          /* Account for the sector of the entry itself */
> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>  {
>      BDRVBlkLogWritesState *s = bs->opaque;
>      QemuOpts *opts;
> -    Error *local_err = NULL;
>      int ret;
>      uint64_t log_sector_size;
>      bool log_append;
> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>          s->nr_entries = 0;
>  
>          if (blk_log_writes_sector_size_valid(log_sector_size)) {
> -            s->cur_log_sector =
> +            int64_t cur_log_sector =
>                  blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
> -            if (local_err) {
> -                ret = -EINVAL;
> -                error_propagate(errp, local_err);
> +                                    le64_to_cpu(log_sb.nr_entries), errp);
> +            if (cur_log_sector < 0) {
> +                ret = cur_log_sector;

This is converting int64_t to int, which is usually not recommended.
In practice this is probably okay because cur_log_sector is supposed
to be a negative errno (ie. an int) in this case.

Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?

>                  goto fail_log;
>              }
>  
> +            s->cur_log_sector = cur_log_sector;
>              s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>          }
>      } else {

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Ari Sundholm]

Hi Vladimir!

Thank you for working on this. My comments below.

On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's simple to avoid error propagation in blk_log_writes_open(), we
> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/blklogwrites.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 7ef046cee9..c7da507b2d 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>           sector_size < (1ull << 24);
>   }
>   
> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> -                                                   uint32_t sector_size,
> -                                                   uint64_t nr_entries,
> -                                                   Error **errp)
> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,

I'd rather not change the return type for reasons detailed below.

> +                                                  uint32_t sector_size,
> +                                                  uint64_t nr_entries,
> +                                                  Error **errp)
>   {
>       uint64_t cur_sector = 1;
>       uint64_t cur_idx = 0;
> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>           if (read_ret < 0) {
>               error_setg_errno(errp, -read_ret,
>                                "Failed to read log entry %"PRIu64, cur_idx);
> -            return (uint64_t)-1ull;
> +            return read_ret;

This is OK, provided the change in return type signedness is necessary 
in the first place.

>           }
>   
>           if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>               error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>                          le64_to_cpu(cur_entry.flags), cur_idx);
> -            return (uint64_t)-1ull;
> +            return -EINVAL;

This is OK, provided the return type signedness change is necessary, 
although we already do have errp to indicate any errors.

>           }
>   
>           /* Account for the sector of the entry itself */
> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>   {
>       BDRVBlkLogWritesState *s = bs->opaque;
>       QemuOpts *opts;
> -    Error *local_err = NULL;
>       int ret;
>       uint64_t log_sector_size;
>       bool log_append;
> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>           s->nr_entries = 0;
>   
>           if (blk_log_writes_sector_size_valid(log_sector_size)) {
> -            s->cur_log_sector =
> +            int64_t cur_log_sector =
>                   blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
> -            if (local_err) {
> -                ret = -EINVAL;
> -                error_propagate(errp, local_err);
> +                                    le64_to_cpu(log_sb.nr_entries), errp);
> +            if (cur_log_sector < 0) {
> +                ret = cur_log_sector;

This changes the semantics slightly. Changing the return type to int64 
may theoretically cause valid return values to be interpreted as errors. 
Since we already do have a dedicated out pointer for the error value 
(errp), why not use it?

Assigning cur_log_sector to ret looks a bit odd to me. Why not use the 
original -EINVAL - or maybe some more appropriate errno value than -EINVAL?

I think the desired effect of this change could be achieved with less 
churn. How about just making just the following change in addition to 
adding ERRP_GUARD() to the beginning of the function and getting rid of 
local_err:

-                                    le64_to_cpu(log_sb.nr_entries), 
&local_err);
+                                    le64_to_cpu(log_sb.nr_entries), errp);
-            if (local_err) {
+            if (*errp) {
                  ret = -EINVAL;
-                error_propagate(errp, local_err);
>                   goto fail_log;
>               }
>   
> +            s->cur_log_sector = cur_log_sector;
>               s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>           }
>       } else {
> 

Best regards,
Ari Sundholm
ari@tuxera.com

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Wed,  9 Sep 2020 21:59:23 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/blklogwrites.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>> 
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>          sector_size < (1ull << 24);
>>  }
>>  
>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> -                                                   uint32_t sector_size,
>> -                                                   uint64_t nr_entries,
>> -                                                   Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> +                                                  uint32_t sector_size,
>> +                                                  uint64_t nr_entries,
>> +                                                  Error **errp)
>>  {
>>      uint64_t cur_sector = 1;
>>      uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>          if (read_ret < 0) {
>>              error_setg_errno(errp, -read_ret,
>>                               "Failed to read log entry %"PRIu64, cur_idx);
>> -            return (uint64_t)-1ull;
>> +            return read_ret;
>
> This changes the error status of blk_log_writes_open() from -EINVAL to
> whatever is returned by bdrv_pread(). I guess this is an improvement
> worth to be mentioned in the changelog.
>
>>          }
>>  
>>          if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>              error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>                         le64_to_cpu(cur_entry.flags), cur_idx);
>> -            return (uint64_t)-1ull;
>> +            return -EINVAL;
>>          }
>>  
>>          /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>  {
>>      BDRVBlkLogWritesState *s = bs->opaque;
>>      QemuOpts *opts;
>> -    Error *local_err = NULL;
>>      int ret;
>>      uint64_t log_sector_size;
>>      bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>          s->nr_entries = 0;
>>  
>>          if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> -            s->cur_log_sector =
>> +            int64_t cur_log_sector =
>>                  blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
>> -            if (local_err) {
>> -                ret = -EINVAL;
>> -                error_propagate(errp, local_err);
>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>> +            if (cur_log_sector < 0) {
>> +                ret = cur_log_sector;
>
> This is converting int64_t to int, which is usually not recommended.
> In practice this is probably okay because cur_log_sector is supposed
> to be a negative errno (ie. an int) in this case.

It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
on error.

Aside: returning uint64_t is awkward.  We commonly use a signed integer
for non-negative offset or negative error.

> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?

Unless we change blk_log_writes_find_cur_log_sector() to return a
negative errno code, we need to ret = -EINVAL here.  Let's keep this
series simple.

>>                  goto fail_log;
>>              }
>>  
>> +            s->cur_log_sector = cur_log_sector;
>>              s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>          }
>>      } else {

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Greg Kurz <groug@kaod.org> writes:
>
>> On Wed,  9 Sep 2020 21:59:23 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>> 
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>  block/blklogwrites.c | 23 +++++++++++------------
>>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 7ef046cee9..c7da507b2d 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>>          sector_size < (1ull << 24);
>>>  }
>>>  
>>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> -                                                   uint32_t sector_size,
>>> -                                                   uint64_t nr_entries,
>>> -                                                   Error **errp)
>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> +                                                  uint32_t sector_size,
>>> +                                                  uint64_t nr_entries,
>>> +                                                  Error **errp)
>>>  {
>>>      uint64_t cur_sector = 1;
>>>      uint64_t cur_idx = 0;
>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>          if (read_ret < 0) {
>>>              error_setg_errno(errp, -read_ret,
>>>                               "Failed to read log entry %"PRIu64, cur_idx);
>>> -            return (uint64_t)-1ull;
>>> +            return read_ret;
>>
>> This changes the error status of blk_log_writes_open() from -EINVAL to
>> whatever is returned by bdrv_pread(). I guess this is an improvement
>> worth to be mentioned in the changelog.
>>
>>>          }
>>>  
>>>          if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>>              error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>>                         le64_to_cpu(cur_entry.flags), cur_idx);
>>> -            return (uint64_t)-1ull;
>>> +            return -EINVAL;
>>>          }
>>>  
>>>          /* Account for the sector of the entry itself */
>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>  {
>>>      BDRVBlkLogWritesState *s = bs->opaque;
>>>      QemuOpts *opts;
>>> -    Error *local_err = NULL;
>>>      int ret;
>>>      uint64_t log_sector_size;
>>>      bool log_append;
>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>          s->nr_entries = 0;
>>>  
>>>          if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>> -            s->cur_log_sector =
>>> +            int64_t cur_log_sector =
>>>                  blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
>>> -            if (local_err) {
>>> -                ret = -EINVAL;
>>> -                error_propagate(errp, local_err);
>>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>>> +            if (cur_log_sector < 0) {
>>> +                ret = cur_log_sector;
>>
>> This is converting int64_t to int, which is usually not recommended.
>> In practice this is probably okay because cur_log_sector is supposed
>> to be a negative errno (ie. an int) in this case.
>
> It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> on error.
>
> Aside: returning uint64_t is awkward.  We commonly use a signed integer
> for non-negative offset or negative error.
>
>> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
>
> Unless we change blk_log_writes_find_cur_log_sector() to return a
> negative errno code,

Which the patch does.  I shouldn't review patches before breakfast.

> negative errno code, we need to ret = -EINVAL here.  Let's keep this
> series simple.

No, the patch is okay as is.

Dumbing it down to keep it simple would also be okay.

Regarding the proposed assertion: do we protect similar conversions from
over-wide negative errno int elsewhere?

>>>                  goto fail_log;
>>>              }
>>>  
>>> +            s->cur_log_sector = cur_log_sector;
>>>              s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>>          }
>>>      } else {

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Greg Kurz]

On Fri, 11 Sep 2020 07:30:37 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Greg Kurz <groug@kaod.org> writes:
> >
> >> On Wed,  9 Sep 2020 21:59:23 +0300
> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >>
> >>> It's simple to avoid error propagation in blk_log_writes_open(), we
> >>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
> >>> 
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>> ---
> >>>  block/blklogwrites.c | 23 +++++++++++------------
> >>>  1 file changed, 11 insertions(+), 12 deletions(-)
> >>> 
> >>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> >>> index 7ef046cee9..c7da507b2d 100644
> >>> --- a/block/blklogwrites.c
> >>> +++ b/block/blklogwrites.c
> >>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
> >>>          sector_size < (1ull << 24);
> >>>  }
> >>>  
> >>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> -                                                   uint32_t sector_size,
> >>> -                                                   uint64_t nr_entries,
> >>> -                                                   Error **errp)
> >>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> +                                                  uint32_t sector_size,
> >>> +                                                  uint64_t nr_entries,
> >>> +                                                  Error **errp)
> >>>  {
> >>>      uint64_t cur_sector = 1;
> >>>      uint64_t cur_idx = 0;
> >>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>>          if (read_ret < 0) {
> >>>              error_setg_errno(errp, -read_ret,
> >>>                               "Failed to read log entry %"PRIu64, cur_idx);
> >>> -            return (uint64_t)-1ull;
> >>> +            return read_ret;
> >>
> >> This changes the error status of blk_log_writes_open() from -EINVAL to
> >> whatever is returned by bdrv_pread(). I guess this is an improvement
> >> worth to be mentioned in the changelog.
> >>
> >>>          }
> >>>  
> >>>          if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> >>>              error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
> >>>                         le64_to_cpu(cur_entry.flags), cur_idx);
> >>> -            return (uint64_t)-1ull;
> >>> +            return -EINVAL;
> >>>          }
> >>>  
> >>>          /* Account for the sector of the entry itself */
> >>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> >>>  {
> >>>      BDRVBlkLogWritesState *s = bs->opaque;
> >>>      QemuOpts *opts;
> >>> -    Error *local_err = NULL;
> >>>      int ret;
> >>>      uint64_t log_sector_size;
> >>>      bool log_append;
> >>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> >>>          s->nr_entries = 0;
> >>>  
> >>>          if (blk_log_writes_sector_size_valid(log_sector_size)) {
> >>> -            s->cur_log_sector =
> >>> +            int64_t cur_log_sector =
> >>>                  blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> >>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
> >>> -            if (local_err) {
> >>> -                ret = -EINVAL;
> >>> -                error_propagate(errp, local_err);
> >>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
> >>> +            if (cur_log_sector < 0) {
> >>> +                ret = cur_log_sector;
> >>
> >> This is converting int64_t to int, which is usually not recommended.
> >> In practice this is probably okay because cur_log_sector is supposed
> >> to be a negative errno (ie. an int) in this case.
> >
> > It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> > on error.
> >
> > Aside: returning uint64_t is awkward.  We commonly use a signed integer
> > for non-negative offset or negative error.
> >
> >> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
> >
> > Unless we change blk_log_writes_find_cur_log_sector() to return a
> > negative errno code,
> 
> Which the patch does.  I shouldn't review patches before breakfast.
> 

:)

> > negative errno code, we need to ret = -EINVAL here.  Let's keep this
> > series simple.
> 
> No, the patch is okay as is.
> 
> Dumbing it down to keep it simple would also be okay.
> 

What about Ari's proposal to add ERRP_GUARD() and check errors
with "if (*errp)" like we do for void functions ?

> Regarding the proposed assertion: do we protect similar conversions from
> over-wide negative errno int elsewhere?
> 

We do protect int64_t->int conversions in a few places, eg.
qcow2_write_snapshots() or qemu_spice_create_host_primary(),
but I couldn't find one about a negarive errno.

> >>>                  goto fail_log;
> >>>              }
> >>>  
> >>> +            s->cur_log_sector = cur_log_sector;
> >>>              s->nr_entries = le64_to_cpu(log_sb.nr_entries);
> >>>          }
> >>>      } else {
> 

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Markus Armbruster]

Ari Sundholm <ari@tuxera.com> writes:

> Hi Vladimir!
>
> Thank you for working on this. My comments below.
>
> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> <vsementsov@virtuozzo.com>
>> ---
>>   block/blklogwrites.c | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>           sector_size < (1ull << 24);
>>   }
>>   -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>> *log,
>> -                                                   uint32_t sector_size,
>> -                                                   uint64_t nr_entries,
>> -                                                   Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>
> I'd rather not change the return type for reasons detailed below.
>
>> +                                                  uint32_t sector_size,
>> +                                                  uint64_t nr_entries,
>> +                                                  Error **errp)
>>   {
>>       uint64_t cur_sector = 1;
>>       uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>           if (read_ret < 0) {
>>               error_setg_errno(errp, -read_ret,
>>                                "Failed to read log entry %"PRIu64, cur_idx);
>> -            return (uint64_t)-1ull;
>> +            return read_ret;
>
> This is OK, provided the change in return type signedness is necessary
> in the first place.
>
>>           }
>>             if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>               error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>                          le64_to_cpu(cur_entry.flags), cur_idx);
>> -            return (uint64_t)-1ull;
>> +            return -EINVAL;
>
> This is OK, provided the return type signedness change is necessary,
> although we already do have errp to indicate any errors.
>
>>           }
>>             /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>   {
>>       BDRVBlkLogWritesState *s = bs->opaque;
>>       QemuOpts *opts;
>> -    Error *local_err = NULL;
>>       int ret;
>>       uint64_t log_sector_size;
>>       bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>           s->nr_entries = 0;
>>             if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> -            s->cur_log_sector =
>> +            int64_t cur_log_sector =
>>                   blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
>> -            if (local_err) {
>> -                ret = -EINVAL;
>> -                error_propagate(errp, local_err);
>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>> +            if (cur_log_sector < 0) {
>> +                ret = cur_log_sector;
>
> This changes the semantics slightly. Changing the return type to int64
> may theoretically cause valid return values to be interpreted as
> errors. Since we already do have a dedicated out pointer for the error
> value (errp), why not use it?

Error.h's big comment:

 * Error reporting system loosely patterned after Glib's GError.
 *
 * = Rules =
 [...]
 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.  Note that
 *   we still have many functions returning void.  We recommend
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.

blk_log_writes_find_cur_log_sector() does return such an error value
before the patch: (uint64_t)-1.

The caller does not use it to check for errors.  It uses @err instead.
Awkward, has to error_propagate().

Avoiding error_propagate() reduces the error handling boileplate.  It
also improves behavior when @errp is &error_abort: we get the abort
right where the error happens instead of where we propagate it.

Furthermore, caller has to make an error code (-EINVAL), because
returning (uint64_t)-1 throws it away.  Yes, a detailed error is stored
into @err, but you can't cleanly extract the error code.

Using a signed integer for returning "non-negative offset or negative
errno code" is pervasive, starting with read() and write().  It hasn't
been a problem there, and it hasn't been a problem in the block layer.
8 exbi-blocks should do for a while.  Should it become troublesome, we
won't solve the problem by going unsigned and adding one bit, we'll
double the width and add 64.

Additional rationale for returning recognizable error values:

commit e3fe3988d7851cac30abffae06d2f555ff7bee62
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Jul 7 18:05:31 2020 +0200

    error: Document Error API usage rules
    
    This merely codifies existing practice, with one exception: the rule
    advising against returning void, where existing practice is mixed.
    
    When the Error API was created, we adopted the (unwritten) rule to
    return void when the function returns no useful value on success,
    unlike GError, which recommends to return true on success and false on
    error then.
    
    When a function returns a distinct error value, say false, a checked
    call that passes the error up looks like
    
        if (!frobnicate(..., errp)) {
            handle the error...
        }
    
    When it returns void, we need
    
        Error *err = NULL;
    
        frobnicate(..., &err);
        if (err) {
            handle the error...
            error_propagate(errp, err);
        }
    
    Not only is this more verbose, it also creates an Error object even
    when @errp is null, &error_abort or &error_fatal.
    
    People got tired of the additional boilerplate, and started to ignore
    the unwritten rule.  The result is confusion among developers about
    the preferred usage.
    
    Make the rule advising against returning void official by putting it
    in writing.  This will hopefully reduce confusion.
    [...]

Additional rationale for avoiding error_propagate():

commit ae7c80a7bd73685437bf6ba9d7c26098351f4166
Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Date:   Tue Jul 7 18:50:30 2020 +0200

    error: New macro ERRP_GUARD()
    
    Introduce a new ERRP_GUARD() macro, to be used at start of functions
    with an errp OUT parameter.
    
    It has three goals:
    
    1. Fix issue with error_fatal and error_prepend/error_append_hint: the
    user can't see this additional information, because exit() happens in
    error_setg earlier than information is added. [Reported by Greg Kurz]
    
    2. Fix issue with error_abort and error_propagate: when we wrap
    error_abort by local_err+error_propagate, the resulting coredump will
    refer to error_propagate and not to the place where error happened.
    (the macro itself doesn't fix the issue, but it allows us to [3.] drop
    the local_err+error_propagate pattern, which will definitely fix the
    issue) [Reported by Kevin Wolf]
    
    3. Drop local_err+error_propagate pattern, which is used to workaround
    void functions with errp parameter, when caller wants to know resulting
    status. (Note: actually these functions could be merely updated to
    return int error code).
    [...]

> Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
> original -EINVAL - or maybe some more appropriate errno value than
> -EINVAL?

We assign the error code returned by
blk_log_writes_find_cur_log_sector(), which seems proper to me.

Care to suggest a better variable name?

> I think the desired effect of this change could be achieved with less
> churn. How about just making just the following change in addition to 
> adding ERRP_GUARD() to the beginning of the function and getting rid
> of local_err:
>
> -                                    le64_to_cpu(log_sb.nr_entries),
>                                      &local_err);
> +                                    le64_to_cpu(log_sb.nr_entries), errp);
> -            if (local_err) {
> +            if (*errp) {
>                  ret = -EINVAL;
> -                error_propagate(errp, local_err);
>>                   goto fail_log;
>>               }
>>   +            s->cur_log_sector = cur_log_sector;
>>               s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>           }
>>       } else {

I prefer this patch.  But it's up to the block maintainers.

Re: [PATCH 08/14] blockjob: return status from block_job_set_speed()

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:24 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  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;
>          }
>      }

Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:25 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

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

... 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 065ec3df0b..ac6a2d3e2a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -967,8 +967,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 8c34b2aef7..9b14c0791f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1090,30 +1090,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);
> @@ -1121,13 +1120,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 10175fa399..eb7c82120c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5056,12 +5056,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;
>          }
>      }
> @@ -5078,9 +5076,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;

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Greg Kurz]

s/startus/status

On Wed,  9 Sep 2020 21:59:27 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  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 e7e662533b..49824be5c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1524,9 +1524,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();

Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
an error_prepend(errp, ...) not visible in the patch context.

Anyway,

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

>      BdrvDirtyBitmap *bitmap;
>      BDRVQcow2State *s = bs->opaque;
>      uint32_t new_nb_bitmaps = s->nb_bitmaps;
> @@ -1546,7 +1547,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;
>          }
>      }
>  
> @@ -1661,7 +1662,7 @@ success:
>      }
>  
>      bitmap_list_free(bm_list);
> -    return;
> +    return true;
>  
>  fail:
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1679,16 +1680,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;
>      }
>  

Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value

[Greg Kurz]

On Wed,  9 Sep 2020 21:59:28 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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>

>  block/qcow2.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c2cd9434cc..31dd28d19e 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;
>      }

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

11.09.2020 12:38, Greg Kurz wrote:
> s/startus/status
> 
> On Wed,  9 Sep 2020 21:59:27 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> It's better to return status together with setting errp. It makes
>> possible to avoid error propagation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   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 e7e662533b..49824be5c6 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1524,9 +1524,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();
> 
> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> an error_prepend(errp, ...) not visible in the patch context.

Ah yes. Actually this is occasional thing I didn't want to include into this patch
(and int this part I). So we can just drop it and leave for part II or part III,
or add a note into commit message

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

Thanks a lot for reviewing :)

Hmm.. With this series I understand the following:

1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:

   - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
   - reviewing is the hardest part of the process

So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.

2. So, the process turns into following steps:

   - apply scripts/coccinelle/errp-guard.cocci
   - look through patches and do obvious refactorings (like this series)
   - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)

3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)

The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).

So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..

> 
>>       BdrvDirtyBitmap *bitmap;
>>       BDRVQcow2State *s = bs->opaque;
>>       uint32_t new_nb_bitmaps = s->nb_bitmaps;
>> @@ -1546,7 +1547,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;
>>           }
>>       }
>>   
>> @@ -1661,7 +1662,7 @@ success:
>>       }
>>   
>>       bitmap_list_free(bm_list);
>> -    return;
>> +    return true;
>>   
>>   fail:
>>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1679,16 +1680,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;
>>       }
>>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Greg Kurz]

On Fri, 11 Sep 2020 13:18:32 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 11.09.2020 12:38, Greg Kurz wrote:
> > s/startus/status
> > 
> > On Wed,  9 Sep 2020 21:59:27 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> > 
> >> It's better to return status together with setting errp. It makes
> >> possible to avoid error propagation.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>   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 e7e662533b..49824be5c6 100644
> >> --- a/block/qcow2.h
> >> +++ b/block/qcow2.h
> >> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -1524,9 +1524,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();
> > 
> > Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> > an error_prepend(errp, ...) not visible in the patch context.
> 
> Ah yes. Actually this is occasional thing I didn't want to include into this patch
> (and int this part I). So we can just drop it and leave for part II or part III,
> or add a note into commit message
> 
> > 
> > Anyway,
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> Thanks a lot for reviewing :)
> 

Don't mention it :)

> Hmm.. With this series I understand the following:
> 
> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
> 
>    - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>    - reviewing is the hardest part of the process
> 
> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
> 
> 2. So, the process turns into following steps:
> 
>    - apply scripts/coccinelle/errp-guard.cocci
>    - look through patches and do obvious refactorings (like this series)
>    - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
> 

I've started to follow this process for the spapr code and, indeed, I
can come up with better changes by refactoring some code manually.
Some of these changes are not that obvious that they could be made
by someone who doesn't know the code, so I tend to agree with your
arguments in 1.

This is also the reason I didn't review patches 10, 13 and 14 because
they looked like I should understand the corresponding code a bit more.

> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
> 

Ha ha :D ... as a consolation prize, maybe we can reach a fair number
of r-b by reviewing each other's _simple_ cleanups ;-)

> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
> 

This sounds nice. My only concern would be to end up fixing code nobody
uses or cares for, so I guess it would be better that active maintainers
or supporters give impetus on that.

> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
> 

I don't :) but the very massive series that were posted on the topic
the last few months look like an announcement to me, at least for
active maintainers and supporters.

> > 
> >>       BdrvDirtyBitmap *bitmap;
> >>       BDRVQcow2State *s = bs->opaque;
> >>       uint32_t new_nb_bitmaps = s->nb_bitmaps;
> >> @@ -1546,7 +1547,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;
> >>           }
> >>       }
> >>   
> >> @@ -1661,7 +1662,7 @@ success:
> >>       }
> >>   
> >>       bitmap_list_free(bm_list);
> >> -    return;
> >> +    return true;
> >>   
> >>   fail:
> >>       QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> >> @@ -1679,16 +1680,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;
> >>       }
> >>   
> > 
> 
> 

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

11.09.2020 14:21, Greg Kurz wrote:
> On Fri, 11 Sep 2020 13:18:32 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 11.09.2020 12:38, Greg Kurz wrote:
>>> s/startus/status
>>>
>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> It's better to return status together with setting errp. It makes
>>>> possible to avoid error propagation.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    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 e7e662533b..49824be5c6 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1524,9 +1524,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();
>>>
>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>> an error_prepend(errp, ...) not visible in the patch context.
>>
>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>> (and int this part I). So we can just drop it and leave for part II or part III,
>> or add a note into commit message
>>
>>>
>>> Anyway,
>>>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> Thanks a lot for reviewing :)
>>
> 
> Don't mention it :)
> 
>> Hmm.. With this series I understand the following:
>>
>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>
>>     - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>     - reviewing is the hardest part of the process
>>
>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> 2. So, the process turns into following steps:
>>
>>     - apply scripts/coccinelle/errp-guard.cocci
>>     - look through patches and do obvious refactorings (like this series)
>>     - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>
> 
> I've started to follow this process for the spapr code and, indeed, I
> can come up with better changes by refactoring some code manually.
> Some of these changes are not that obvious that they could be made
> by someone who doesn't know the code, so I tend to agree with your
> arguments in 1.
> 
> This is also the reason I didn't review patches 10, 13 and 14 because
> they looked like I should understand the corresponding code a bit more.
> 
>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
>>
> 
> Ha ha :D ... as a consolation prize, maybe we can reach a fair number
> of r-b by reviewing each other's _simple_ cleanups ;-)
> 
>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
>>
> 
> This sounds nice. My only concern would be to end up fixing code nobody
> uses or cares for, so I guess it would be better that active maintainers
> or supporters give impetus on that.
> 
>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
> 
> I don't :) but the very massive series that were posted on the topic
> the last few months look like an announcement to me, at least for
> active maintainers and supporters.

Aha, I know. Better than announcement is improving checkpatch.

> 
>>>
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        BDRVQcow2State *s = bs->opaque;
>>>>        uint32_t new_nb_bitmaps = s->nb_bitmaps;
>>>> @@ -1546,7 +1547,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;
>>>>            }
>>>>        }
>>>>    
>>>> @@ -1661,7 +1662,7 @@ success:
>>>>        }
>>>>    
>>>>        bitmap_list_free(bm_list);
>>>> -    return;
>>>> +    return true;
>>>>    
>>>>    fail:
>>>>        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> @@ -1679,16 +1680,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;
>>>>        }
>>>>    
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Fri, 11 Sep 2020 07:30:37 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> No, the patch is okay as is.
>> 
>> Dumbing it down to keep it simple would also be okay.
>> 
>
> What about Ari's proposal to add ERRP_GUARD() and check errors
> with "if (*errp)" like we do for void functions ?

Up to the maintainer.  I prefer this patch.

>> Regarding the proposed assertion: do we protect similar conversions from
>> over-wide negative errno int elsewhere?
>> 
>
> We do protect int64_t->int conversions in a few places, eg.
> qcow2_write_snapshots() or qemu_spice_create_host_primary(),
> but I couldn't find one about a negarive errno.

Then I'd not protect it here, either.

>> >>>                  goto fail_log;
>> >>>              }
>> >>>  
>> >>> +            s->cur_log_sector = cur_log_sector;
>> >>>              s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> >>>          }
>> >>>      } else {
>> 

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 11.09.2020 14:21, Greg Kurz wrote:
>> On Fri, 11 Sep 2020 13:18:32 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 
>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>> s/startus/status
>>>>
>>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>>> It's better to return status together with setting errp. It makes
>>>>> possible to avoid error propagation.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    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 e7e662533b..49824be5c6 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>>> --- a/block/qcow2-bitmap.c
>>>>> +++ b/block/qcow2-bitmap.c
>>>>> @@ -1524,9 +1524,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();
>>>>
>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>
>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>> or add a note into commit message

Either works for me.

>>>>
>>>> Anyway,
>>>>
>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>
>>> Thanks a lot for reviewing :)
>>>
>> Don't mention it :)
>> 
>>> Hmm.. With this series I understand the following:
>>>
>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>
>>>     - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>     - reviewing is the hardest part of the process
>>>
>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.

Yes, going the extra mile is better.

I recommend it for code that is actively maintained.  Making the code
simpler and thus easier to maintain is an investment that'll pay off.

We may have code where it won't pay off.  Do you think a blind
application of errp-guard.cocci might be better than nothing there?

>>> 2. So, the process turns into following steps:
>>>
>>>     - apply scripts/coccinelle/errp-guard.cocci
>>>     - look through patches and do obvious refactorings (like this series)
>>>     - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>
>> I've started to follow this process for the spapr code and, indeed,
>> I
>> can come up with better changes by refactoring some code manually.
>> Some of these changes are not that obvious that they could be made
>> by someone who doesn't know the code, so I tend to agree with your
>> arguments in 1.
>> This is also the reason I didn't review patches 10, 13 and 14
>> because
>> they looked like I should understand the corresponding code a bit more.
>> 
>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)

LOL

A lesser craftsman than you would've peddled the generated commits
anyway.  Props!

>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>> number
>> of r-b by reviewing each other's _simple_ cleanups ;-)
>> 
>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).

If people care enough about [2] to submit patches, I'll feel obliged to
help merging them.

>> This sounds nice. My only concern would be to end up fixing code
>> nobody
>> uses or cares for, so I guess it would be better that active maintainers
>> or supporters give impetus on that.

A bit of weeding on the side is always appreciated, but please don't
feel obliged to sink lots of time into code you don't care about.

>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>
>> I don't :) but the very massive series that were posted on the topic
>> the last few months look like an announcement to me, at least for
>> active maintainers and supporters.
>
> Aha, I know. Better than announcement is improving checkpatch.

Yes, automated feedback works best.

Relentless pressure from reviewers can also work in the long, long run.
But it's tiresome.

Of course, checkpatch.pl checks only new or changed code.  Any ideas on
how to make people aware of the opportunity to simplify their existing
code?  Obvious: posting more patches simplifying existing code we care
about.  Any other smart ideas?

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Vladimir Sementsov-Ogievskiy]

11.09.2020 18:22, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 11.09.2020 14:21, Greg Kurz wrote:
>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>>> s/startus/status
>>>>>
>>>>> On Wed,  9 Sep 2020 21:59:27 +0300
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>
>>>>>> It's better to return status together with setting errp. It makes
>>>>>> possible to avoid error propagation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     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 e7e662533b..49824be5c6 100644
>>>>>> --- a/block/qcow2.h
>>>>>> +++ b/block/qcow2.h
>>>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>>>> --- a/block/qcow2-bitmap.c
>>>>>> +++ b/block/qcow2-bitmap.c
>>>>>> @@ -1524,9 +1524,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();
>>>>>
>>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>>
>>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>>> or add a note into commit message
> 
> Either works for me.
> 
>>>>>
>>>>> Anyway,
>>>>>
>>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>>
>>>> Thanks a lot for reviewing :)
>>>>
>>> Don't mention it :)
>>>
>>>> Hmm.. With this series I understand the following:
>>>>
>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>
>>>>      - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>>      - reviewing is the hardest part of the process
>>>>
>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
> 
> Yes, going the extra mile is better.
> 
> I recommend it for code that is actively maintained.  Making the code
> simpler and thus easier to maintain is an investment that'll pay off.
> 
> We may have code where it won't pay off.  Do you think a blind
> application of errp-guard.cocci might be better than nothing there?

I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
So, let's start from popular subsystems. And make a decision for the rest later.

> 
>>>> 2. So, the process turns into following steps:
>>>>
>>>>      - apply scripts/coccinelle/errp-guard.cocci
>>>>      - look through patches and do obvious refactorings (like this series)
>>>>      - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>>
>>> I've started to follow this process for the spapr code and, indeed,
>>> I
>>> can come up with better changes by refactoring some code manually.
>>> Some of these changes are not that obvious that they could be made
>>> by someone who doesn't know the code, so I tend to agree with your
>>> arguments in 1.
>>> This is also the reason I didn't review patches 10, 13 and 14
>>> because
>>> they looked like I should understand the corresponding code a bit more.
>>>
>>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
> 
> LOL
> 
> A lesser craftsman than you would've peddled the generated commits
> anyway.  Props!
> 
>>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>>> number
>>> of r-b by reviewing each other's _simple_ cleanups ;-)
>>>
>>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
> 
> If people care enough about [2] to submit patches, I'll feel obliged to
> help merging them.
> 
>>> This sounds nice. My only concern would be to end up fixing code
>>> nobody
>>> uses or cares for, so I guess it would be better that active maintainers
>>> or supporters give impetus on that.
> 
> A bit of weeding on the side is always appreciated, but please don't
> feel obliged to sink lots of time into code you don't care about.
> 
>>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
>>> I don't :) but the very massive series that were posted on the topic
>>> the last few months look like an announcement to me, at least for
>>> active maintainers and supporters.
>>
>> Aha, I know. Better than announcement is improving checkpatch.
> 
> Yes, automated feedback works best.
> 
> Relentless pressure from reviewers can also work in the long, long run.
> But it's tiresome.
> 
> Of course, checkpatch.pl checks only new or changed code.  Any ideas on
> how to make people aware of the opportunity to simplify their existing
> code?  Obvious: posting more patches simplifying existing code we care
> about.  Any other smart ideas?
> 

I think it would be great for checkpatch to warn about some code problems in function touched by checked patch. Code-style, error-api, etc. This will attract developers to improve existing code they are interested in.


-- 
Best regards,
Vladimir

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 11.09.2020 18:22, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 11.09.2020 14:21, Greg Kurz wrote:
>>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
[...]
>>>>> Hmm.. With this series I understand the following:
>>>>>
>>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>>
>>>>>      - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>>>      - reviewing is the hardest part of the process
>>>>>
>>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> Yes, going the extra mile is better.
>>
>> I recommend it for code that is actively maintained.  Making the code
>> simpler and thus easier to maintain is an investment that'll pay off.
>>
>> We may have code where it won't pay off.  Do you think a blind
>> application of errp-guard.cocci might be better than nothing there?
>
> I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
> So, let's start from popular subsystems. And make a decision for the rest later.

Makes sense.

[...]

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Ari Sundholm]

Hi,

On 9/11/20 11:03 AM, Markus Armbruster wrote:
> Ari Sundholm <ari@tuxera.com> writes:
> 
>> Hi Vladimir!
>>
>> Thank you for working on this. My comments below.
>>
>> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com>
>>> ---
>>>    block/blklogwrites.c | 23 +++++++++++------------
>>>    1 file changed, 11 insertions(+), 12 deletions(-)
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 7ef046cee9..c7da507b2d 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>>            sector_size < (1ull << 24);
>>>    }
>>>    -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>>> *log,
>>> -                                                   uint32_t sector_size,
>>> -                                                   uint64_t nr_entries,
>>> -                                                   Error **errp)
>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>
>> I'd rather not change the return type for reasons detailed below.
>>
>>> +                                                  uint32_t sector_size,
>>> +                                                  uint64_t nr_entries,
>>> +                                                  Error **errp)
>>>    {
>>>        uint64_t cur_sector = 1;
>>>        uint64_t cur_idx = 0;
>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>            if (read_ret < 0) {
>>>                error_setg_errno(errp, -read_ret,
>>>                                 "Failed to read log entry %"PRIu64, cur_idx);
>>> -            return (uint64_t)-1ull;
>>> +            return read_ret;
>>
>> This is OK, provided the change in return type signedness is necessary
>> in the first place.
>>
>>>            }
>>>              if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>>                error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>>                           le64_to_cpu(cur_entry.flags), cur_idx);
>>> -            return (uint64_t)-1ull;
>>> +            return -EINVAL;
>>
>> This is OK, provided the return type signedness change is necessary,
>> although we already do have errp to indicate any errors.
>>
>>>            }
>>>              /* Account for the sector of the entry itself */
>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>    {
>>>        BDRVBlkLogWritesState *s = bs->opaque;
>>>        QemuOpts *opts;
>>> -    Error *local_err = NULL;
>>>        int ret;
>>>        uint64_t log_sector_size;
>>>        bool log_append;
>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>            s->nr_entries = 0;
>>>              if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>> -            s->cur_log_sector =
>>> +            int64_t cur_log_sector =
>>>                    blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
>>> -            if (local_err) {
>>> -                ret = -EINVAL;
>>> -                error_propagate(errp, local_err);
>>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>>> +            if (cur_log_sector < 0) {
>>> +                ret = cur_log_sector;
>>
>> This changes the semantics slightly. Changing the return type to int64
>> may theoretically cause valid return values to be interpreted as
>> errors. Since we already do have a dedicated out pointer for the error
>> value (errp), why not use it?
> 
> Error.h's big comment:
> 
>   * Error reporting system loosely patterned after Glib's GError.
>   *
>   * = Rules =
>   [...]
>   * - Whenever practical, also return a value that indicates success /
>   *   failure.  This can make the error checking more concise, and can
>   *   avoid useless error object creation and destruction.  Note that
>   *   we still have many functions returning void.  We recommend
>   *   • bool-valued functions return true on success / false on failure,
>   *   • pointer-valued functions return non-null / null pointer, and
>   *   • integer-valued functions return non-negative / negative.
> 
> blk_log_writes_find_cur_log_sector() does return such an error value
> before the patch: (uint64_t)-1.
> 
> The caller does not use it to check for errors.  It uses @err instead.
> Awkward, has to error_propagate().
> 
> Avoiding error_propagate() reduces the error handling boileplate.  It
> also improves behavior when @errp is &error_abort: we get the abort
> right where the error happens instead of where we propagate it.
> 
> Furthermore, caller has to make an error code (-EINVAL), because
> returning (uint64_t)-1 throws it away.  Yes, a detailed error is stored
> into @err, but you can't cleanly extract the error code.
> 
> Using a signed integer for returning "non-negative offset or negative
> errno code" is pervasive, starting with read() and write().  It hasn't
> been a problem there, and it hasn't been a problem in the block layer.
> 8 exbi-blocks should do for a while.  Should it become troublesome, we
> won't solve the problem by going unsigned and adding one bit, we'll
> double the width and add 64.
> 

I am in complete agreement with eliminating error propagation within the 
blklogwrites driver. This was never a point of disagreement.

As error propagation is dropped in this patch, the awkwardness referred 
to above will be no more, making that a moot point.

My main issue was that this patch does more than just the mechanical 
transformation required. Changes that are not strictly necessary are 
made, and they slightly change the semantics while duplicating the error 
code and halving the range of the return type (instead of just returning 
*some* value in the absence of a possibility to return nothing, which 
will be thrown away by the caller anyway when an error has occurred).

However, as the consensus seems to be that it is best to change the 
return type to int64_t for consistency with the rest of the codebase, I 
will not object any further regarding that point. Having conceded that, 
this makes the difference between my preferred minimalistic approach and 
this patch insignificant, and it is not my intention to needlessly 
obstruct a perfectly fine patch series.

BTW, I do not think it would be difficult at all to extend the code in 
error.h to make it convenient to extract errno and/or win32 error values 
that have been explicitly provided, but this is a matter best discussed 
separately and left for a later patch.

As for general matters regarding error handling and separation between 
results and errors, I am open to discussing these off-list.

Best regards,
Ari Sundholm
ari@tuxera.com

> Additional rationale for returning recognizable error values:
> 
> commit e3fe3988d7851cac30abffae06d2f555ff7bee62
> Author: Markus Armbruster <armbru@redhat.com>
> Date:   Tue Jul 7 18:05:31 2020 +0200
> 
>      error: Document Error API usage rules
>      
>      This merely codifies existing practice, with one exception: the rule
>      advising against returning void, where existing practice is mixed.
>      
>      When the Error API was created, we adopted the (unwritten) rule to
>      return void when the function returns no useful value on success,
>      unlike GError, which recommends to return true on success and false on
>      error then.
>      
>      When a function returns a distinct error value, say false, a checked
>      call that passes the error up looks like
>      
>          if (!frobnicate(..., errp)) {
>              handle the error...
>          }
>      
>      When it returns void, we need
>      
>          Error *err = NULL;
>      
>          frobnicate(..., &err);
>          if (err) {
>              handle the error...
>              error_propagate(errp, err);
>          }
>      
>      Not only is this more verbose, it also creates an Error object even
>      when @errp is null, &error_abort or &error_fatal.
>      
>      People got tired of the additional boilerplate, and started to ignore
>      the unwritten rule.  The result is confusion among developers about
>      the preferred usage.
>      
>      Make the rule advising against returning void official by putting it
>      in writing.  This will hopefully reduce confusion.
>      [...]
> 
> Additional rationale for avoiding error_propagate():
> 
> commit ae7c80a7bd73685437bf6ba9d7c26098351f4166
> Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Date:   Tue Jul 7 18:50:30 2020 +0200
> 
>      error: New macro ERRP_GUARD()
>      
>      Introduce a new ERRP_GUARD() macro, to be used at start of functions
>      with an errp OUT parameter.
>      
>      It has three goals:
>      
>      1. Fix issue with error_fatal and error_prepend/error_append_hint: the
>      user can't see this additional information, because exit() happens in
>      error_setg earlier than information is added. [Reported by Greg Kurz]
>      
>      2. Fix issue with error_abort and error_propagate: when we wrap
>      error_abort by local_err+error_propagate, the resulting coredump will
>      refer to error_propagate and not to the place where error happened.
>      (the macro itself doesn't fix the issue, but it allows us to [3.] drop
>      the local_err+error_propagate pattern, which will definitely fix the
>      issue) [Reported by Kevin Wolf]
>      
>      3. Drop local_err+error_propagate pattern, which is used to workaround
>      void functions with errp parameter, when caller wants to know resulting
>      status. (Note: actually these functions could be merely updated to
>      return int error code).
>      [...]
> 
>> Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
>> original -EINVAL - or maybe some more appropriate errno value than
>> -EINVAL?
> 
> We assign the error code returned by
> blk_log_writes_find_cur_log_sector(), which seems proper to me.
> 
> Care to suggest a better variable name?
> 
>> I think the desired effect of this change could be achieved with less
>> churn. How about just making just the following change in addition to
>> adding ERRP_GUARD() to the beginning of the function and getting rid
>> of local_err:
>>
>> -                                    le64_to_cpu(log_sb.nr_entries),
>>                                       &local_err);
>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>> -            if (local_err) {
>> +            if (*errp) {
>>                   ret = -EINVAL;
>> -                error_propagate(errp, local_err);
>>>                    goto fail_log;
>>>                }
>>>    +            s->cur_log_sector = cur_log_sector;
>>>                s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>>            }
>>>        } else {
> 
> I prefer this patch.  But it's up to the block maintainers.
> 

Re: [PATCH 07/14] block/blklogwrites: drop error propagation

[Markus Armbruster]

Ari Sundholm <ari@tuxera.com> writes:

> Hi,
>
> On 9/11/20 11:03 AM, Markus Armbruster wrote:
>> Ari Sundholm <ari@tuxera.com> writes:
>> 
>>> Hi Vladimir!
>>>
>>> Thank you for working on this. My comments below.
>>>
>>> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>> <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/blklogwrites.c | 23 +++++++++++------------
>>>>    1 file changed, 11 insertions(+), 12 deletions(-)
>>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>>> index 7ef046cee9..c7da507b2d 100644
>>>> --- a/block/blklogwrites.c
>>>> +++ b/block/blklogwrites.c
>>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>>>            sector_size < (1ull << 24);
>>>>    }
>>>>    -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>>>> *log,
>>>> -                                                   uint32_t sector_size,
>>>> -                                                   uint64_t nr_entries,
>>>> -                                                   Error **errp)
>>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>
>>> I'd rather not change the return type for reasons detailed below.
>>>
>>>> +                                                  uint32_t sector_size,
>>>> +                                                  uint64_t nr_entries,
>>>> +                                                  Error **errp)
>>>>    {
>>>>        uint64_t cur_sector = 1;
>>>>        uint64_t cur_idx = 0;
>>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>>            if (read_ret < 0) {
>>>>                error_setg_errno(errp, -read_ret,
>>>>                                 "Failed to read log entry %"PRIu64, cur_idx);
>>>> -            return (uint64_t)-1ull;
>>>> +            return read_ret;
>>>
>>> This is OK, provided the change in return type signedness is necessary
>>> in the first place.
>>>
>>>>            }
>>>>              if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>>>                error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>>>                           le64_to_cpu(cur_entry.flags), cur_idx);
>>>> -            return (uint64_t)-1ull;
>>>> +            return -EINVAL;
>>>
>>> This is OK, provided the return type signedness change is necessary,
>>> although we already do have errp to indicate any errors.
>>>
>>>>            }
>>>>              /* Account for the sector of the entry itself */
>>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>>    {
>>>>        BDRVBlkLogWritesState *s = bs->opaque;
>>>>        QemuOpts *opts;
>>>> -    Error *local_err = NULL;
>>>>        int ret;
>>>>        uint64_t log_sector_size;
>>>>        bool log_append;
>>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>>            s->nr_entries = 0;
>>>>              if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>>> -            s->cur_log_sector =
>>>> +            int64_t cur_log_sector =
>>>>                    blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>>> -                                    le64_to_cpu(log_sb.nr_entries), &local_err);
>>>> -            if (local_err) {
>>>> -                ret = -EINVAL;
>>>> -                error_propagate(errp, local_err);
>>>> +                                    le64_to_cpu(log_sb.nr_entries), errp);
>>>> +            if (cur_log_sector < 0) {
>>>> +                ret = cur_log_sector;
>>>
>>> This changes the semantics slightly. Changing the return type to int64
>>> may theoretically cause valid return values to be interpreted as
>>> errors. Since we already do have a dedicated out pointer for the error
>>> value (errp), why not use it?
>> Error.h's big comment:
>>   * Error reporting system loosely patterned after Glib's GError.
>>   *
>>   * = Rules =
>>   [...]
>>   * - Whenever practical, also return a value that indicates success /
>>   *   failure.  This can make the error checking more concise, and can
>>   *   avoid useless error object creation and destruction.  Note that
>>   *   we still have many functions returning void.  We recommend
>>   *   • bool-valued functions return true on success / false on failure,
>>   *   • pointer-valued functions return non-null / null pointer, and
>>   *   • integer-valued functions return non-negative / negative.
>> blk_log_writes_find_cur_log_sector() does return such an error value
>> before the patch: (uint64_t)-1.
>> The caller does not use it to check for errors.  It uses @err
>> instead.
>> Awkward, has to error_propagate().
>> Avoiding error_propagate() reduces the error handling boileplate.
>> It
>> also improves behavior when @errp is &error_abort: we get the abort
>> right where the error happens instead of where we propagate it.
>> Furthermore, caller has to make an error code (-EINVAL), because
>> returning (uint64_t)-1 throws it away.  Yes, a detailed error is stored
>> into @err, but you can't cleanly extract the error code.
>> Using a signed integer for returning "non-negative offset or
>> negative
>> errno code" is pervasive, starting with read() and write().  It hasn't
>> been a problem there, and it hasn't been a problem in the block layer.
>> 8 exbi-blocks should do for a while.  Should it become troublesome, we
>> won't solve the problem by going unsigned and adding one bit, we'll
>> double the width and add 64.
>> 
>
> I am in complete agreement with eliminating error propagation within
> the blklogwrites driver. This was never a point of disagreement.
>
> As error propagation is dropped in this patch, the awkwardness
> referred to above will be no more, making that a moot point.
>
> My main issue was that this patch does more than just the mechanical
> transformation required. Changes that are not strictly necessary are 
> made, and they slightly change the semantics while duplicating the
> error code and halving the range of the return type (instead of just
> returning *some* value in the absence of a possibility to return
> nothing, which will be thrown away by the caller anyway when an error
> has occurred).

You have a point: the return type change should perhaps be a separate
patch.  Up to the maintainer.

> However, as the consensus seems to be that it is best to change the
> return type to int64_t for consistency with the rest of the codebase,
> I will not object any further regarding that point. Having conceded
> that, this makes the difference between my preferred minimalistic
> approach and this patch insignificant, and it is not my intention to
> needlessly obstruct a perfectly fine patch series.
>
> BTW, I do not think it would be difficult at all to extend the code in
> error.h to make it convenient to extract errno and/or win32 error
> values that have been explicitly provided, but this is a matter best
> discussed separately and left for a later patch.

Certainly feasible, but how would we deal with the fact that only some
Error objects have an error code?

> As for general matters regarding error handling and separation between
> results and errors, I am open to discussing these off-list.

Thanks!

Re: [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:20 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:21 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_set_backing_hd now returns status, let's use it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

Berto

Re: [PATCH 02/14] block: use return status of bdrv_append()

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 08/14] blockjob: return status from block_job_set_speed()

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:24 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 01/14] block: return status from bdrv_append and friends

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:17 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:19 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

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

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:22 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:27 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  [...]
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>                                            bool release_stored, Error **errp)
>  {
> +    ERRP_GUARD();

This needs an explanation.

Berto

Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:28 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 1. Drop extra error propagation
>
> 2. 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 qcow2_do_open to not behave this
> way. This allows to simplify code in qcow2_co_invalidate_cache().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 31dd28d19e..cc4e7dd461 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>                                        int flags, Error **errp)
>  {
> +    ERRP_GUARD();

Why is this necessary?

>      BDRVQcow2State *s = bs->opaque;
>      unsigned int len, i;
>      int ret = 0;
> @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          report_unsupported_feature(errp, feature_table,
>                                     s->incompatible_features &
>                                     ~QCOW2_INCOMPAT_MASK);
> +        error_setg(errp,
> +                   "qcow2 header contains unknown
>      incompatible_feature bits");

I think that this is a mistake because the previous call to
report_unsupported_feature() already calls error_setg();

> @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
>  static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>                                                     Error **errp)
>  {
> +    ERRP_GUARD();

Again, why is this necessary?

Berto

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

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:30 PM CEST, 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>

Berto

Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> + * On success return true with bm_list set (probably to NULL, if no bitmaps),

" probably " ? :-)

> + * 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;

So here 'list' points at NULL and 'plist' at &list.

> -        *plist = obj;
> -        plist = &obj->next;

In the original code 'plist' is updated when you add a new element, so
it always points at the end of the list. But 'list' is unchanged and it
still points at the first element.

So the caller receives a pointer to the first element.

> +        *info_list = obj;
> +        info_list = &obj->next;

But in the new code there is only one variable (passed by the caller),
which always points at the end of the list.

Berto

Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Alberto Garcia]

On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> -/* 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.
> - */
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +/* Return true on success, false on failure. */
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> +                              Error **errp)

I think that the documentation should clarify under what conditions
'header_updated' is modified.

>      if (s->nb_bitmaps == 0) {
>          /* No bitmaps - nothing to do */
> -        return false;
> +        return true;
>      }

Here is it not for example (should it be set to false?).

> -    if (bm_list == NULL) {
> +    if (!bm_list) {
>          return false;
>      }

This looks like a cosmetic change unrelated to the rest of the patch.

Berto

Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:23, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 1. Drop extra error propagation
>>
>> 2. 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 qcow2_do_open to not behave this
>> way. This allows to simplify code in qcow2_co_invalidate_cache().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 16 +++++++---------
>>   1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 31dd28d19e..cc4e7dd461 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
>>   static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>                                         int flags, Error **errp)
>>   {
>> +    ERRP_GUARD();
> 
> Why is this necessary?

Because error_append_hint() used in the function. Without ERRP_GUARD, error_append_hint won't work if errp = &error_fatal
Read more in include/qapi/error.h near ERRP_GUARD definition.

But yes, it's good to not it in commit message.

> 
>>       BDRVQcow2State *s = bs->opaque;
>>       unsigned int len, i;
>>       int ret = 0;
>> @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>           report_unsupported_feature(errp, feature_table,
>>                                      s->incompatible_features &
>>                                      ~QCOW2_INCOMPAT_MASK);
>> +        error_setg(errp,
>> +                   "qcow2 header contains unknown
>>       incompatible_feature bits");
> 
> I think that this is a mistake because the previous call to
> report_unsupported_feature() already calls error_setg();

Oops, you are right.

> 
>> @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
>>   static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>>                                                      Error **errp)
>>   {
>> +    ERRP_GUARD();
> 
> Again, why is this necessary?
> 

Because it uses error_prepend() after conversion (same reason as for error_append_hint()).

Thanks for review! I'll post v2 soon.

-- 
Best regards,
Vladimir

Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:32, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
> 
> " probably " ? :-)

I note this as "set to NULL" is not obvious thing (is it "unset" ? :).. And by "probably" I mean "may be", i.e. NULL is just one of possible cases. Probably I use "probably" in a wrong way?

> 
>> + * 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;
> 
> So here 'list' points at NULL and 'plist' at &list.

Hmm, to be precise, list _is_ NULL (and points nowhere), and plist points to list.

> 
>> -        *plist = obj;
>> -        plist = &obj->next;
> 
> In the original code 'plist' is updated when you add a new element, so
> it always points at the end of the list. But 'list' is unchanged and it
> still points at the first element.
> 
> So the caller receives a pointer to the first element.
> 
>> +        *info_list = obj;
>> +        info_list = &obj->next;
> 
> But in the new code there is only one variable (passed by the caller),
> which always points at the end of the list.
> 

No: at first "*info_list = obj", we set the result which user will get, users pointer now points to the first object in the list.
Then, at "info_list = &obj->next", we reassign info_list to another pointer: to "next" field of first list item. So, all further "*info_list = obj" are note visible to the caller.

Actually, the logic is not changed, just instead of plist we use info_list, and instead of list - a variable which should be defined in the caller. Look: in old code, first "*plist = obj" sets "list" variable, but all further "*plist = obj" don't change "list" variable.


-- 
Best regards,
Vladimir

Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface

[Vladimir Sementsov-Ogievskiy]

17.09.2020 19:35, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> -/* 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.
>> - */
>> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> +/* Return true on success, false on failure. */
>> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
>> +                              Error **errp)
> 
> I think that the documentation should clarify under what conditions
> 'header_updated' is modified.
> 
>>       if (s->nb_bitmaps == 0) {
>>           /* No bitmaps - nothing to do */
>> -        return false;
>> +        return true;
>>       }
> 
> Here is it not for example (should it be set to false?).

Ha, I think, it just shows that patch is wrong :) We should set header_updated at least on every success path. Or better always (if it is non-NULL of course). Thanks for careful review!

> 
>> -    if (bm_list == NULL) {
>> +    if (!bm_list) {
>>           return false;
>>       }
> 
> This looks like a cosmetic change unrelated to the rest of the patch.
> 
> Berto
> 


-- 
Best regards,
Vladimir

Re: [PATCH 02/14] block: use return status of bdrv_append()

[Vladimir Sementsov-Ogievskiy]

10.09.2020 19:10, Greg Kurz wrote:
> On Wed,  9 Sep 2020 21:59:18 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 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>
> 
> Just one suggestion for a follow-up below...
> 
>>   block.c                     |  5 +----
>>   block/backup-top.c          | 20 ++++++++------------

[..]

>> @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>>       CommitBlockJob *s;
>>       BlockDriverState *iter;
>>       BlockDriverState *commit_top_bs = NULL;
>> -    Error *local_err = NULL;
>>       int ret;
>>   
> 
> ... this is unrelated but while reviewing I've noticed that the ret
> variable isn't really needed.
> 

Looking at this now, I'm not quite agreed. I think that avoiding multi-line if conditions is a good reason for additional variables, like this ret. A kind of taste of course, so if you want you may post a patch, but I don't want do it :)

-- 
Best regards,
Vladimir