* [PATCH v2 00/13] block: deal with errp: part I
@ 2020-09-17 19:55 Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
` (13 more replies)
0 siblings, 14 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
v2:
01-07: add Greg's and Alberto's r-bs
08: fix wording in commit message
add Greg's r-b
09: fix header_updated logic, add comment, drop unrelated style-change [Alberto]
10: - fix commit header
- add note about ERRP_GUARD in commit message
- add Greg's r-b
11: add Greg's and Alberto's r-bs
12: was "[PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp"
- drop wrong update of qcow2_do_open() (and ERRP_GUARD() fix
for qcow2_do_open becomes unrelated, drop it too, will
update later)
- reword commit message correspondingly
13: add Alberto's r-b
"[PATCH 07/14] block/blklogwrites: drop error propagation" is dropped
for now, as it needs separate small series.
Vladimir Sementsov-Ogievskiy (13):
block: return status from bdrv_append and friends
block: use return status of bdrv_append()
block: check return value of bdrv_open_child and drop error
propagation
blockdev: fix drive_backup_prepare() missed error
block: drop extra error propagation for bdrv_set_backing_hd
block/mirror: drop extra error propagation in commit_active_start()
blockjob: return status from block_job_set_speed()
block/qcow2: qcow2_get_specific_info(): drop error propagation
block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
block/qcow2-bitmap: return status from
qcow2_store_persistent_dirty_bitmaps
block/qcow2: read_cache_sizes: return status value
block/qcow2: simplify qcow2_co_invalidate_cache()
block/qed: bdrv_qed_do_open: deal with errp
block/qcow2.h | 9 ++---
include/block/block.h | 12 +++----
include/block/blockjob.h | 2 +-
block.c | 50 +++++++++++++++-------------
block/backup-top.c | 20 +++++-------
block/blkdebug.c | 6 ++--
block/blklogwrites.c | 10 +++---
block/blkreplay.c | 6 ++--
block/blkverify.c | 11 +++----
block/commit.c | 5 +--
block/mirror.c | 18 ++++------
block/qcow2-bitmap.c | 65 +++++++++++++++++++------------------
block/qcow2.c | 53 ++++++++++++------------------
block/qed.c | 24 +++++++++-----
block/quorum.c | 6 ++--
blockdev.c | 7 ++--
blockjob.c | 18 +++++-----
tests/test-bdrv-graph-mod.c | 6 ++--
18 files changed, 150 insertions(+), 178 deletions(-)
--
2.21.3
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 01/13] block: return status from bdrv_append and friends
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
The recommended use of qemu error api assumes returning status together
with setting errp and avoid void functions with errp parameter. Let's
improve bdrv_append and some friends to reduce error-propagation
overhead in further patches.
Choose int return status, because bdrv_replace_node() has call to
bdrv_check_update_perm(), which reports int status, which seems correct
to propagate.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 12 ++++++------
block.c | 39 ++++++++++++++++++++++++---------------
2 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 981ab5b314..a997dbf95b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -336,10 +336,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
BlockDriverState *bdrv_new(void);
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp);
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp);
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+ Error **errp);
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp);
int bdrv_parse_aio(const char *mode, int *flags);
int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
@@ -351,8 +351,8 @@ BdrvChild *bdrv_open_child(const char *filename,
BdrvChildRole child_role,
bool allow_none, Error **errp);
BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp);
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
BlockDriverState *bdrv_open(const char *filename, const char *reference,
diff --git a/block.c b/block.c
index 9538af4884..f922c6d8f4 100644
--- a/block.c
+++ b/block.c
@@ -2869,14 +2869,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
* Sets the bs->backing link of a BDS. A new reference is created; callers
* which don't need their own reference any more must call bdrv_unref().
*/
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp)
{
+ int ret = 0;
bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
bdrv_inherits_from_recursive(backing_hd, bs);
if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
- return;
+ return -EPERM;
}
if (backing_hd) {
@@ -2895,15 +2896,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
bdrv_backing_role(bs), errp);
+ if (!bs->backing) {
+ ret = -EINVAL;
+ goto out;
+ }
+
/* If backing_hd was already part of bs's backing chain, and
* inherits_from pointed recursively to bs then let's update it to
* point directly to bs (else it will become NULL). */
- if (bs->backing && update_inherits_from) {
+ if (update_inherits_from) {
backing_hd->inherits_from = bs;
}
out:
bdrv_refresh_limits(bs, NULL);
+
+ return ret;
}
/*
@@ -4553,8 +4561,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
return ret;
}
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp)
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp)
{
BdrvChild *c, *next;
GSList *list = NULL, *p;
@@ -4576,6 +4584,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
continue;
}
if (c->frozen) {
+ ret = -EPERM;
error_setg(errp, "Cannot change '%s' link to '%s'",
c->name, from->node_name);
goto out;
@@ -4611,6 +4620,8 @@ out:
g_slist_free(list);
bdrv_drained_end(from);
bdrv_unref(from);
+
+ return ret;
}
/*
@@ -4629,20 +4640,16 @@ out:
* parents of bs_top after bdrv_append() returns. If the caller needs to keep a
* reference of its own, it must call bdrv_ref().
*/
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp)
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+ Error **errp)
{
- Error *local_err = NULL;
-
- bdrv_set_backing_hd(bs_new, bs_top, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
+ if (ret < 0) {
goto out;
}
- bdrv_replace_node(bs_top, bs_new, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ ret = bdrv_replace_node(bs_top, bs_new, errp);
+ if (ret < 0) {
bdrv_set_backing_hd(bs_new, NULL, &error_abort);
goto out;
}
@@ -4651,6 +4658,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
* additional reference any more. */
out:
bdrv_unref(bs_new);
+
+ return ret;
}
static void bdrv_delete(BlockDriverState *bs)
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 02/13] block: use return status of bdrv_append()
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 03/13] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Now bdrv_append returns status and we can drop all the local_err things
around it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 5 +----
block/backup-top.c | 20 ++++++++------------
block/commit.c | 5 +----
block/mirror.c | 6 ++----
blockdev.c | 4 +---
tests/test-bdrv-graph-mod.c | 6 +++---
6 files changed, 16 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index f922c6d8f4..b4e36d6dd7 100644
--- a/block.c
+++ b/block.c
@@ -3160,7 +3160,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
- Error *local_err = NULL;
int ret;
/* if snapshot, we create a temporary backing file and open it
@@ -3207,9 +3206,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
* order to be able to return one, we have to increase
* bs_snapshot's refcount here */
bdrv_ref(bs_snapshot);
- bdrv_append(bs_snapshot, bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (bdrv_append(bs_snapshot, bs, errp) < 0) {
bs_snapshot = NULL;
goto out;
}
diff --git a/block/backup-top.c b/block/backup-top.c
index fe6883cc97..eb6a34b726 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,7 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
BlockCopyState **bcs,
Error **errp)
{
- Error *local_err = NULL;
+ ERRP_GUARD();
BDRVBackupTopState *state;
BlockDriverState *top;
bool appended = false;
@@ -223,9 +223,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
bdrv_drained_begin(source);
bdrv_ref(top);
- bdrv_append(top, source, &local_err);
- if (local_err) {
- error_prepend(&local_err, "Cannot append backup-top filter: ");
+ if (bdrv_append(top, source, errp) < 0) {
+ error_prepend(errp, "Cannot append backup-top filter: ");
goto fail;
}
appended = true;
@@ -235,18 +234,16 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
* we want.
*/
state->active = true;
- bdrv_child_refresh_perms(top, top->backing, &local_err);
- if (local_err) {
- error_prepend(&local_err,
- "Cannot set permissions for backup-top filter: ");
+ if (bdrv_child_refresh_perms(top, top->backing, errp) < 0) {
+ error_prepend(errp, "Cannot set permissions for backup-top filter: ");
goto fail;
}
state->cluster_size = cluster_size;
state->bcs = block_copy_state_new(top->backing, state->target,
- cluster_size, write_flags, &local_err);
- if (local_err) {
- error_prepend(&local_err, "Cannot create block-copy-state: ");
+ cluster_size, write_flags, errp);
+ if (!state->bcs) {
+ error_prepend(errp, "Cannot create block-copy-state: ");
goto fail;
}
*bcs = state->bcs;
@@ -264,7 +261,6 @@ fail:
}
bdrv_drained_end(source);
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/block/commit.c b/block/commit.c
index 1e85c306cc..6da0902f9d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -254,7 +254,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
BlockDriverState *filtered_base;
- Error *local_err = NULL;
int64_t base_size, top_size;
uint64_t base_perms, iter_shared_perms;
int ret;
@@ -312,10 +311,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
commit_top_bs->total_sectors = top->total_sectors;
- bdrv_append(commit_top_bs, top, &local_err);
- if (local_err) {
+ if (bdrv_append(commit_top_bs, top, errp) < 0) {
commit_top_bs = NULL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/mirror.c b/block/mirror.c
index 26acf4af6f..b3778248b8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1560,7 +1560,6 @@ static BlockJob *mirror_start_job(
BlockDriverState *mirror_top_bs;
bool target_is_backing;
uint64_t target_perms, target_shared_perms;
- Error *local_err = NULL;
int ret;
if (granularity == 0) {
@@ -1609,12 +1608,11 @@ static BlockJob *mirror_start_job(
* it alive until block_job_create() succeeds even if bs has no parent. */
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
- bdrv_append(mirror_top_bs, bs, &local_err);
+ ret = bdrv_append(mirror_top_bs, bs, errp);
bdrv_drained_end(bs);
- if (local_err) {
+ if (ret < 0) {
bdrv_unref(mirror_top_bs);
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/blockdev.c b/blockdev.c
index 7f2561081e..b9803e553f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1581,9 +1581,7 @@ static void external_snapshot_prepare(BlkActionState *common,
* can fail, so we need to do it in .prepare; undoing it for abort is
* always possible. */
bdrv_ref(state->new_bs);
- bdrv_append(state->new_bs, state->old_bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (bdrv_append(state->new_bs, state->old_bs, errp) < 0) {
goto out;
}
state->overlay_appended = true;
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index 8cff13830e..2b71601c24 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
*/
static void test_update_perm_tree(void)
{
- Error *local_err = NULL;
+ int ret;
BlockBackend *root = blk_new(qemu_get_aio_context(),
BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
@@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
bdrv_attach_child(filter, bs, "child", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
- bdrv_append(filter, bs, &local_err);
- error_free_or_abort(&local_err);
+ ret = bdrv_append(filter, bs, NULL);
+ assert(ret < 0);
blk_unref(root);
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 03/13] block: check return value of bdrv_open_child and drop error propagation
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 04/13] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
This patch is generated by cocci script:
@@
symbol bdrv_open_child, errp, local_err;
expression file;
@@
file = bdrv_open_child(...,
- &local_err
+ errp
);
- if (local_err)
+ if (!file)
{
...
- error_propagate(errp, local_err);
...
}
with command
spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80 --use-gitgrep block
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/blkdebug.c | 6 ++----
block/blklogwrites.c | 10 ++++------
block/blkreplay.c | 6 ++----
block/blkverify.c | 11 ++++-------
block/qcow2.c | 5 ++---
block/quorum.c | 6 ++----
6 files changed, 16 insertions(+), 28 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index eecbf3e5c4..5716b817ae 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -464,7 +464,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVBlkdebugState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
uint64_t align;
@@ -494,10 +493,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto out;
}
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 13ae63983b..b7579370a3 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the file */
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false,
- &local_err);
- if (local_err) {
+ errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
/* Open the log file */
s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_of_bds,
- BDRV_CHILD_METADATA, false, &local_err);
- if (local_err) {
+ BDRV_CHILD_METADATA, false, errp);
+ if (!s->log_file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 30a0f5d57a..4a247752fd 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -23,16 +23,14 @@ typedef struct Request {
static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
- Error *local_err = NULL;
int ret;
/* Open the image file */
bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkverify.c b/block/blkverify.c
index 4aed53ab59..95ae73e2aa 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVBlkverifyState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
/* Open the test file */
s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
"test", bs, &child_of_bds, BDRV_CHILD_DATA,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!s->test_file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index b05512718c..41a29072e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1612,9 +1612,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* Open external data file */
s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
&child_of_bds, BDRV_CHILD_DATA,
- true, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ true, errp);
+ if (!s->data_file) {
ret = -EINVAL;
goto fail;
}
diff --git a/block/quorum.c b/block/quorum.c
index e846a7e892..2ebe0ba16d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -900,7 +900,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVQuorumState *s = bs->opaque;
- Error *local_err = NULL;
QemuOpts *opts = NULL;
const char *pattern_str;
bool *opened;
@@ -978,9 +977,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
&child_of_bds, BDRV_CHILD_DATA, false,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (!s->children[i]) {
ret = -EINVAL;
goto close_exit;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 04/13] blockdev: fix drive_backup_prepare() missed error
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 03/13] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 05/13] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
We leak local_err and don't report failure to the caller. It's
definitely wrong, let's fix.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index b9803e553f..d6bde81ad4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1813,8 +1813,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
aio_context_acquire(aio_context);
if (set_backing_hd) {
- bdrv_set_backing_hd(target_bs, source, &local_err);
- if (local_err) {
+ if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
goto unref;
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 05/13] block: drop extra error propagation for bdrv_set_backing_hd
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 04/13] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 06/13] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
bdrv_set_backing_hd now returns status, let's use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index b4e36d6dd7..1cf825c349 100644
--- a/block.c
+++ b/block.c
@@ -3015,11 +3015,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
- bdrv_set_backing_hd(bs, backing_hd, &local_err);
+ ret = bdrv_set_backing_hd(bs, backing_hd, errp);
bdrv_unref(backing_hd);
- if (local_err) {
- error_propagate(errp, local_err);
- ret = -EINVAL;
+ if (ret < 0) {
goto free_exit;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 06/13] block/mirror: drop extra error propagation in commit_active_start()
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 05/13] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 07/13] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Let's check return value of mirror_start_job to check for failure
instead of local_err.
Rename ret to job, as ret is usually integer variable.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/mirror.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index b3778248b8..f7c624d6a9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1851,8 +1851,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
bool auto_complete, Error **errp)
{
bool base_read_only;
- Error *local_err = NULL;
- BlockJob *ret;
+ BlockJob *job;
base_read_only = bdrv_is_read_only(base);
@@ -1862,19 +1861,18 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
}
}
- ret = mirror_start_job(
+ job = mirror_start_job(
job_id, bs, creation_flags, base, NULL, speed, 0, 0,
MIRROR_LEAVE_BACKING_CHAIN, false,
on_error, on_error, true, cb, opaque,
&commit_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (!job) {
goto error_restore_flags;
}
- return ret;
+ return job;
error_restore_flags:
/* ignore error and errp for bdrv_reopen, because we want to propagate
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 07/13] blockjob: return status from block_job_set_speed()
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 06/13] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Better to return status together with setting errp. It allows to avoid
error propagation in the caller.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/blockjob.h | 2 +-
blockjob.c | 18 ++++++++----------
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 35faa3aa26..d200f33c10 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
* Set a rate-limiting parameter for the job; the actual meaning may
* vary depending on the job type.
*/
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
/**
* block_job_query:
diff --git a/blockjob.c b/blockjob.c
index 470facfd47..afddf7a1fb 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job)
return timer_pending(&job->sleep_timer);
}
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
{
int64_t old_speed = job->speed;
- if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) {
- return;
+ if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
+ return false;
}
if (speed < 0) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
"a non-negative value");
- return;
+ return false;
}
ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME);
job->speed = speed;
if (speed && speed <= old_speed) {
- return;
+ return true;
}
/* kick only if a timer is pending */
job_enter_cond(&job->job, job_timer_pending);
+
+ return true;
}
int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
@@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
/* Only set speed when necessary to avoid NotSupported error */
if (speed != 0) {
- Error *local_err = NULL;
-
- block_job_set_speed(job, speed, &local_err);
- if (local_err) {
+ if (!block_job_set_speed(job, speed, errp)) {
job_early_fail(&job->job);
- error_propagate(errp, local_err);
return NULL;
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 07/13] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-18 15:04 ` Alberto Garcia
2020-09-17 19:55 ` [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface is rather
weird.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
block/qcow2.h | 4 ++--
block/qcow2-bitmap.c | 27 +++++++++++++--------------
block/qcow2.c | 10 +++-------
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index b71e444fca..6eac088f1c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -973,8 +973,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
- Error **errp);
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+ Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d7a31a8ddc..4f6138f544 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1093,30 +1093,29 @@ static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
/*
* qcow2_get_bitmap_info_list()
* Returns a list of QCOW2 bitmap details.
- * In case of no bitmaps, the function returns NULL and
- * the @errp parameter is not set.
- * When bitmap information can not be obtained, the function returns
- * NULL and the @errp parameter is set.
+ * On success return true with bm_list set (probably to NULL, if no bitmaps),
+ * on failure return false with errp set.
*/
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
- Error **errp)
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+ Qcow2BitmapInfoList **info_list, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
- Qcow2BitmapInfoList *list = NULL;
- Qcow2BitmapInfoList **plist = &list;
if (s->nb_bitmaps == 0) {
- return NULL;
+ *info_list = NULL;
+ return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
- return NULL;
+ if (!bm_list) {
+ return false;
}
+ *info_list = NULL;
+
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
@@ -1124,13 +1123,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
info->name = g_strdup(bm->name);
info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
obj->value = info;
- *plist = obj;
- plist = &obj->next;
+ *info_list = obj;
+ info_list = &obj->next;
}
bitmap_list_free(bm_list);
- return list;
+ return true;
}
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
diff --git a/block/qcow2.c b/block/qcow2.c
index 41a29072e6..8c89c98978 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5038,12 +5038,10 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
- Error *local_err = NULL;
if (s->crypto != NULL) {
- encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ encrypt_info = qcrypto_block_get_info(s->crypto, errp);
+ if (!encrypt_info) {
return NULL;
}
}
@@ -5060,9 +5058,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
};
} else if (s->qcow_version == 3) {
Qcow2BitmapInfoList *bitmaps;
- bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) {
qapi_free_ImageInfoSpecific(spec_info);
qapi_free_QCryptoBlockInfo(encrypt_info);
return NULL;
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-18 14:54 ` Alberto Garcia
2020-09-18 15:15 ` Greg Kurz
2020-09-17 19:55 ` [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
13 siblings, 2 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
It's recommended for bool functions with errp to return true on success
and false on failure. Non-standard interfaces don't help to understand
the code. The change is also needed to reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 3 ++-
block/qcow2-bitmap.c | 25 ++++++++++++++-----------
block/qcow2.c | 6 ++----
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 6eac088f1c..3c64dcda33 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 4f6138f544..500175f4e8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -962,25 +962,26 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
}
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
+/*
+ * Return true on success, false on failure. Anyway, if header_updated
+ * provided set it appropriately.
*/
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
GSList *created_dirty_bitmaps = NULL;
- bool header_updated = false;
bool needs_update = false;
+ if (header_updated) {
+ *header_updated = false;
+ }
+
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
- return false;
+ return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
@@ -1036,7 +1037,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
error_setg_errno(errp, -ret, "Can't update bitmap directory");
goto fail;
}
- header_updated = true;
+ if (header_updated) {
+ *header_updated = true;
+ }
}
if (!can_write(bs)) {
@@ -1047,7 +1050,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
g_slist_free(created_dirty_bitmaps);
bitmap_list_free(bm_list);
- return header_updated;
+ return true;
fail:
g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c89c98978..c4b86df7c0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
unsigned int len, i;
int ret = 0;
QCowHeader header;
- Error *local_err = NULL;
uint64_t ext_end;
uint64_t l1_vm_state_index;
bool update_header = false;
@@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
- bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
+ bool header_updated;
+ if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-18 15:21 ` Alberto Garcia
2020-09-17 19:55 ` [PATCH v2 11/13] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
13 siblings, 1 reply; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
It's better to return status together with setting errp. It makes
possible to avoid error propagation.
While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
above ERRP_GUARD() definition in include/qapi/error.h)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
block/qcow2.h | 2 +-
block/qcow2-bitmap.c | 13 ++++++-------
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 3c64dcda33..7884a5088d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -978,7 +978,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 500175f4e8..b8ff347885 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1534,9 +1534,10 @@ out:
* readonly to begin with, and whether we opened directly or reopened to that
* state shouldn't matter for the state we get afterward.
*/
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp)
{
+ ERRP_GUARD();
BdrvDirtyBitmap *bitmap;
BDRVQcow2State *s = bs->opaque;
uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1556,7 +1557,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
- return;
+ return false;
}
}
@@ -1671,7 +1672,7 @@ success:
}
bitmap_list_free(bm_list);
- return;
+ return true;
fail:
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1689,16 +1690,14 @@ fail:
}
bitmap_list_free(bm_list);
+ return false;
}
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
{
BdrvDirtyBitmap *bitmap;
- Error *local_err = NULL;
- qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
+ if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
return -EINVAL;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 11/13] block/qcow2: read_cache_sizes: return status value
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
13 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
It's better to return status together with setting errp. It allows to
reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c4b86df7c0..2b6ec4b757 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs,
cache_clean_timer_init(bs, new_context);
}
-static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
+static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
uint64_t *l2_cache_size,
uint64_t *l2_cache_entry_size,
uint64_t *refcount_cache_size, Error **errp)
@@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
" and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
"at the same time");
- return;
+ return false;
} else if (l2_cache_size_set &&
(l2_cache_max_setting > combined_cache_size)) {
error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
- return;
+ return false;
} else if (*refcount_cache_size > combined_cache_size) {
error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
- return;
+ return false;
}
if (l2_cache_size_set) {
@@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
error_setg(errp, "L2 cache entry size must be a power of two "
"between %d and the cluster size (%d)",
1 << MIN_CLUSTER_BITS, s->cluster_size);
- return;
+ return false;
}
+
+ return true;
}
typedef struct Qcow2ReopenState {
@@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
int i;
const char *encryptfmt;
QDict *encryptopts = NULL;
- Error *local_err = NULL;
int ret;
qdict_extract_subqdict(options, &encryptopts, "encrypt.");
@@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
}
/* get L2 table/refcount block cache size from command line options */
- read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
- &refcount_cache_size, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
+ &refcount_cache_size, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 11/13] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-18 15:26 ` Alberto Garcia
2020-09-18 15:30 ` Greg Kurz
2020-09-17 19:55 ` [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
2020-09-17 20:15 ` [PATCH v2 00/13] block: deal with errp: part I no-reply
13 siblings, 2 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
qcow2_do_open correctly sets errp on each failure path. So, we can
simplify code in qcow2_co_invalidate_cache() and drop explicit error
propagation. We should use ERRP_GUARD() (accordingly to comment in
include/qapi/error.h) together with error_append() call which we add to
avoid problems with error_fatal.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 2b6ec4b757..cd5f48d3fb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
{
+ ERRP_GUARD();
BDRVQcow2State *s = bs->opaque;
int flags = s->flags;
QCryptoBlock *crypto = NULL;
QDict *options;
- Error *local_err = NULL;
int ret;
/*
@@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
flags &= ~BDRV_O_INACTIVE;
qemu_co_mutex_lock(&s->lock);
- ret = qcow2_do_open(bs, options, flags, &local_err);
+ ret = qcow2_do_open(bs, options, flags, errp);
qemu_co_mutex_unlock(&s->lock);
qobject_unref(options);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "Could not reopen qcow2 layer: ");
- bs->drv = NULL;
- return;
- } else if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
+ if (ret < 0) {
+ error_prepend(errp, "Could not reopen qcow2 layer: ");
bs->drv = NULL;
return;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
@ 2020-09-17 19:55 ` Vladimir Sementsov-Ogievskiy
2020-09-18 16:07 ` Greg Kurz
2020-09-17 20:15 ` [PATCH v2 00/13] block: deal with errp: part I no-reply
13 siblings, 1 reply; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:55 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Set errp always on failure. Generic bdrv_open_driver supports driver
functions which can return negative value and forget to set errp.
That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
this way. This allows to simplify code in
bdrv_qed_co_invalidate_cache().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qed.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/block/qed.c b/block/qed.c
index b27e7546ca..f45c640513 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
if (ret < 0) {
+ error_setg(errp, "Failed to read QED header");
return ret;
}
qed_header_le_to_cpu(&le_header, &s->header);
@@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
return -ENOTSUP;
}
if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
+ error_setg(errp, "QED cluster size is invalid");
return -EINVAL;
}
/* Round down file size to the last cluster */
file_size = bdrv_getlength(bs->file->bs);
if (file_size < 0) {
+ error_setg(errp, "Failed to get file length");
return file_size;
}
s->file_size = qed_start_of_cluster(s, file_size);
if (!qed_is_table_size_valid(s->header.table_size)) {
+ error_setg(errp, "QED table size is invalid");
return -EINVAL;
}
if (!qed_is_image_size_valid(s->header.image_size,
s->header.cluster_size,
s->header.table_size)) {
+ error_setg(errp, "QED image size is invalid");
return -EINVAL;
}
if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+ error_setg(errp, "QED table offset is invalid");
return -EINVAL;
}
@@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
/* Header size calculation must not overflow uint32_t */
if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
+ error_setg(errp, "QED header size is too large");
return -EINVAL;
}
@@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
if ((uint64_t)s->header.backing_filename_offset +
s->header.backing_filename_size >
s->header.cluster_size * s->header.header_size) {
+ error_setg(errp, "QED backing filename offset is invalid");
return -EINVAL;
}
@@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
bs->auto_backing_file,
sizeof(bs->auto_backing_file));
if (ret < 0) {
+ error_setg(errp, "Failed to read backing filename");
return ret;
}
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
@@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
ret = qed_write_header_sync(s);
if (ret) {
+ error_setg(errp, "Failed to update header");
return ret;
}
@@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
ret = qed_read_l1_table_sync(s);
if (ret) {
+ error_setg(errp, "Failed to read L1 table");
goto out;
}
@@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
ret = qed_check(s, &result, true);
if (ret) {
+ error_setg(errp, "Image corrupted");
goto out;
}
}
@@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
{
BDRVQEDState *s = bs->opaque;
- Error *local_err = NULL;
int ret;
bdrv_qed_close(bs);
bdrv_qed_init_state(bs);
qemu_co_mutex_lock(&s->table_lock);
- ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
+ ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
qemu_co_mutex_unlock(&s->table_lock);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "Could not reopen qed layer: ");
- return;
- } else if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not reopen qed layer");
- return;
+ if (ret < 0) {
+ error_prepend(errp, "Could not reopen qed layer: ");
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 00/13] block: deal with errp: part I
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2020-09-17 19:55 ` [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
@ 2020-09-17 20:15 ` no-reply
2020-09-18 12:27 ` Vladimir Sementsov-Ogievskiy
13 siblings, 1 reply; 27+ messages in thread
From: no-reply @ 2020-09-17 20:15 UTC (permalink / raw)
To: vsementsov
Cc: kwolf, vsementsov, berto, pavel.dovgaluk, qemu-block, jsnow,
qemu-devel, armbru, groug, stefanha, pbonzini, mreitz, ari
Patchew URL: https://patchew.org/QEMU/20200917195519.19589-1-vsementsov@virtuozzo.com/
Hi,
This series failed build test on FreeBSD host. Please find the details below.
The full log is available at
http://patchew.org/logs/20200917195519.19589-1-vsementsov@virtuozzo.com/testing.FreeBSD/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 00/13] block: deal with errp: part I
2020-09-17 20:15 ` [PATCH v2 00/13] block: deal with errp: part I no-reply
@ 2020-09-18 12:27 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-18 12:27 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, armbru, berto, eblake, jsnow, stefanha, pbonzini,
pavel.dovgaluk, ari, mreitz, kwolf, groug
17.09.2020 23:15, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200917195519.19589-1-vsementsov@virtuozzo.com/
>
>
>
> Hi,
>
> This series failed build test on FreeBSD host. Please find the details below.
>
>
>
>
>
>
> The full log is available at
> http://patchew.org/logs/20200917195519.19589-1-vsementsov@virtuozzo.com/testing.FreeBSD/?type=message.
Link is broken, it shows: "N/A. Internal error while reading log file"
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-17 19:55 ` [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
@ 2020-09-18 14:54 ` Alberto Garcia
2020-09-18 14:59 ` Vladimir Sementsov-Ogievskiy
2020-09-18 15:15 ` Greg Kurz
1 sibling, 1 reply; 27+ messages in thread
From: Alberto Garcia @ 2020-09-18 14:54 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's recommended for bool functions with errp to return true on success
> and false on failure. Non-standard interfaces don't help to understand
> the code. The change is also needed to reduce error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> +/*
> + * Return true on success, false on failure. Anyway, if header_updated
> + * provided set it appropriately.
> */
I'm not a native speaker but it sounds a bit odd to me. Maybe "If
header_updated is not NULL then it is set appropriately regardless of
the return value".
But I'm fine with your version, so
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-18 14:54 ` Alberto Garcia
@ 2020-09-18 14:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-18 14:59 UTC (permalink / raw)
To: Alberto Garcia, qemu-block
Cc: qemu-devel, armbru, eblake, jsnow, stefanha, pbonzini,
pavel.dovgaluk, ari, mreitz, kwolf, groug
18.09.2020 17:54, Alberto Garcia wrote:
> On Thu 17 Sep 2020 09:55:15 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> It's recommended for bool functions with errp to return true on success
>> and false on failure. Non-standard interfaces don't help to understand
>> the code. The change is also needed to reduce error propagation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> +/*
>> + * Return true on success, false on failure. Anyway, if header_updated
>> + * provided set it appropriately.
>> */
>
> I'm not a native speaker but it sounds a bit odd to me. Maybe "If
> header_updated is not NULL then it is set appropriately regardless of
> the return value".
That's better I think, thanks.
>
> But I'm fine with your version, so
>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
>
> Berto
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-17 19:55 ` [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-18 15:04 ` Alberto Garcia
0 siblings, 0 replies; 27+ messages in thread
From: Alberto Garcia @ 2020-09-18 15:04 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu 17 Sep 2020 09:55:14 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> Don't use error propagation in qcow2_get_specific_info(). For this
> refactor qcow2_get_bitmap_info_list, its current interface is rather
> weird.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> - * In case of no bitmaps, the function returns NULL and
> - * the @errp parameter is not set.
> - * When bitmap information can not be obtained, the function returns
> - * NULL and the @errp parameter is set.
> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
> + * on failure return false with errp set.
I still find the 'probably' thing odd :-) I think the API documentation
should describe what the function does and under which conditions, not
the probability of the outcomes.
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
> Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
> @@ -1124,13 +1123,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> info->name = g_strdup(bm->name);
> info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
> obj->value = info;
> - *plist = obj;
> - plist = &obj->next;
> + *info_list = obj;
> + info_list = &obj->next;
> }
You were right with this, I got confused by the fact that you are
modifying the pointer provided by the user.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-17 19:55 ` [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
2020-09-18 14:54 ` Alberto Garcia
@ 2020-09-18 15:15 ` Greg Kurz
1 sibling, 0 replies; 27+ messages in thread
From: Greg Kurz @ 2020-09-18 15:15 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu, 17 Sep 2020 22:55:15 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> It's recommended for bool functions with errp to return true on success
> and false on failure. Non-standard interfaces don't help to understand
> the code. The change is also needed to reduce error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
With the documentation change suggested by Berto,
Reviewed-by: Greg Kurz <groug@kaod.org>
> block/qcow2.h | 3 ++-
> block/qcow2-bitmap.c | 25 ++++++++++++++-----------
> block/qcow2.c | 6 ++----
> 3 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 6eac088f1c..3c64dcda33 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
> int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> void **refcount_table,
> int64_t *refcount_table_size);
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> + Error **errp);
> bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
> Qcow2BitmapInfoList **info_list, Error **errp);
> int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 4f6138f544..500175f4e8 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -962,25 +962,26 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
> bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
> }
>
> -/* qcow2_load_dirty_bitmaps()
> - * Return value is a hint for caller: true means that the Qcow2 header was
> - * updated. (false doesn't mean that the header should be updated by the
> - * caller, it just means that updating was not needed or the image cannot be
> - * written to).
> - * On failure the function returns false.
> +/*
> + * Return true on success, false on failure. Anyway, if header_updated
> + * provided set it appropriately.
> */
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> + Error **errp)
> {
> BDRVQcow2State *s = bs->opaque;
> Qcow2BitmapList *bm_list;
> Qcow2Bitmap *bm;
> GSList *created_dirty_bitmaps = NULL;
> - bool header_updated = false;
> bool needs_update = false;
>
> + if (header_updated) {
> + *header_updated = false;
> + }
> +
> if (s->nb_bitmaps == 0) {
> /* No bitmaps - nothing to do */
> - return false;
> + return true;
> }
>
> bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> @@ -1036,7 +1037,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> error_setg_errno(errp, -ret, "Can't update bitmap directory");
> goto fail;
> }
> - header_updated = true;
> + if (header_updated) {
> + *header_updated = true;
> + }
> }
>
> if (!can_write(bs)) {
> @@ -1047,7 +1050,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> g_slist_free(created_dirty_bitmaps);
> bitmap_list_free(bm_list);
>
> - return header_updated;
> + return true;
>
> fail:
> g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8c89c98978..c4b86df7c0 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> unsigned int len, i;
> int ret = 0;
> QCowHeader header;
> - Error *local_err = NULL;
> uint64_t ext_end;
> uint64_t l1_vm_state_index;
> bool update_header = false;
> @@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>
> if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
> /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
> - bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
> - if (local_err != NULL) {
> - error_propagate(errp, local_err);
> + bool header_updated;
> + if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
> ret = -EINVAL;
> goto fail;
> }
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps
2020-09-17 19:55 ` [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
@ 2020-09-18 15:21 ` Alberto Garcia
0 siblings, 0 replies; 27+ messages in thread
From: Alberto Garcia @ 2020-09-18 15:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu 17 Sep 2020 09:55:16 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
> usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
> above ERRP_GUARD() definition in include/qapi/error.h)
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-17 19:55 ` [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
@ 2020-09-18 15:26 ` Alberto Garcia
2020-09-18 15:30 ` Greg Kurz
1 sibling, 0 replies; 27+ messages in thread
From: Alberto Garcia @ 2020-09-18 15:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu 17 Sep 2020 09:55:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation. We should use ERRP_GUARD() (accordingly to comment in
> include/qapi/error.h) together with error_append() call which we add to
> avoid problems with error_fatal.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-17 19:55 ` [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
2020-09-18 15:26 ` Alberto Garcia
@ 2020-09-18 15:30 ` Greg Kurz
2020-09-18 15:51 ` Alberto Garcia
1 sibling, 1 reply; 27+ messages in thread
From: Greg Kurz @ 2020-09-18 15:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu, 17 Sep 2020 22:55:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation. We should use ERRP_GUARD() (accordingly to comment in
> include/qapi/error.h) together with error_append() call which we add to
> avoid problems with error_fatal.
>
The wording gives the impression that we add error_append() to avoid problems
with error_fatal which is certainly not true. Also it isn't _append() but
_prepend() :)
What about ?
"Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
to avoid problems with the error_prepend() call if errp is &error_fatal."
With that fixed,
Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2b6ec4b757..cd5f48d3fb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
> static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> Error **errp)
> {
> + ERRP_GUARD();
> BDRVQcow2State *s = bs->opaque;
> int flags = s->flags;
> QCryptoBlock *crypto = NULL;
> QDict *options;
> - Error *local_err = NULL;
> int ret;
>
> /*
> @@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>
> flags &= ~BDRV_O_INACTIVE;
> qemu_co_mutex_lock(&s->lock);
> - ret = qcow2_do_open(bs, options, flags, &local_err);
> + ret = qcow2_do_open(bs, options, flags, errp);
> qemu_co_mutex_unlock(&s->lock);
> qobject_unref(options);
> - if (local_err) {
> - error_propagate_prepend(errp, local_err,
> - "Could not reopen qcow2 layer: ");
> - bs->drv = NULL;
> - return;
> - } else if (ret < 0) {
> - error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
> + if (ret < 0) {
> + error_prepend(errp, "Could not reopen qcow2 layer: ");
> bs->drv = NULL;
> return;
> }
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-18 15:30 ` Greg Kurz
@ 2020-09-18 15:51 ` Alberto Garcia
2020-09-18 16:01 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 27+ messages in thread
From: Alberto Garcia @ 2020-09-18 15:51 UTC (permalink / raw)
To: Greg Kurz, Vladimir Sementsov-Ogievskiy
Cc: kwolf, pavel.dovgaluk, qemu-block, qemu-devel, armbru, stefanha,
pbonzini, mreitz, jsnow, ari
On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
>> qcow2_do_open correctly sets errp on each failure path. So, we can
>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
>> propagation. We should use ERRP_GUARD() (accordingly to comment in
>> include/qapi/error.h) together with error_append() call which we add to
>> avoid problems with error_fatal.
>>
>
> The wording gives the impression that we add error_append() to avoid problems
> with error_fatal which is certainly not true. Also it isn't _append() but
> _prepend() :)
>
> What about ?
>
> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
> to avoid problems with the error_prepend() call if errp is
> &error_fatal."
I had to go to the individual error functions to see what "it doesn't
work with &error_fatal" actually means.
So in a case like qcow2_do_open() which has:
error_setg(errp, ...)
error_append_hint(errp, ...)
As far as I can see this works just fine without ERRP_GUARD() and with
error_fatal, the difference is that if we don't use the guard then the
process exists during error_setg(), and if we use the guard it exists
during the implicit error_propagate() call triggered by its destruction
at the end of the function. In this latter case the printed error
message would include the hint.
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-18 15:51 ` Alberto Garcia
@ 2020-09-18 16:01 ` Vladimir Sementsov-Ogievskiy
2020-09-18 16:10 ` Greg Kurz
0 siblings, 1 reply; 27+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-18 16:01 UTC (permalink / raw)
To: Alberto Garcia, Greg Kurz
Cc: qemu-block, qemu-devel, armbru, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf
18.09.2020 18:51, Alberto Garcia wrote:
> On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
>>> qcow2_do_open correctly sets errp on each failure path. So, we can
>>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
>>> propagation. We should use ERRP_GUARD() (accordingly to comment in
>>> include/qapi/error.h) together with error_append() call which we add to
>>> avoid problems with error_fatal.
>>>
>>
>> The wording gives the impression that we add error_append() to avoid problems
>> with error_fatal which is certainly not true. Also it isn't _append() but
>> _prepend() :)
>>
>> What about ?
>>
>> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
>> to avoid problems with the error_prepend() call if errp is
>> &error_fatal."
OK for me.
>
> I had to go to the individual error functions to see what "it doesn't
> work with &error_fatal" actually means.
>
> So in a case like qcow2_do_open() which has:
>
> error_setg(errp, ...)
> error_append_hint(errp, ...)
>
> As far as I can see this works just fine without ERRP_GUARD() and with
> error_fatal, the difference is that if we don't use the guard then the
> process exists during error_setg(), and if we use the guard it exists
> during the implicit error_propagate() call triggered by its destruction
> at the end of the function. In this latter case the printed error
> message would include the hint.
>
Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp
2020-09-17 19:55 ` [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
@ 2020-09-18 16:07 ` Greg Kurz
0 siblings, 0 replies; 27+ messages in thread
From: Greg Kurz @ 2020-09-18 16:07 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Thu, 17 Sep 2020 22:55:19 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Set errp always on failure. Generic bdrv_open_driver supports driver
> functions which can return negative value and forget to set errp.
> That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
> this way. This allows to simplify code in
> bdrv_qed_co_invalidate_cache().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> block/qed.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/block/qed.c b/block/qed.c
> index b27e7546ca..f45c640513 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>
> ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
> if (ret < 0) {
> + error_setg(errp, "Failed to read QED header");
> return ret;
> }
> qed_header_le_to_cpu(&le_header, &s->header);
> @@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
> return -ENOTSUP;
> }
> if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
> + error_setg(errp, "QED cluster size is invalid");
> return -EINVAL;
> }
>
> /* Round down file size to the last cluster */
> file_size = bdrv_getlength(bs->file->bs);
> if (file_size < 0) {
> + error_setg(errp, "Failed to get file length");
> return file_size;
> }
> s->file_size = qed_start_of_cluster(s, file_size);
>
> if (!qed_is_table_size_valid(s->header.table_size)) {
> + error_setg(errp, "QED table size is invalid");
> return -EINVAL;
> }
> if (!qed_is_image_size_valid(s->header.image_size,
> s->header.cluster_size,
> s->header.table_size)) {
> + error_setg(errp, "QED image size is invalid");
> return -EINVAL;
> }
> if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
> + error_setg(errp, "QED table offset is invalid");
> return -EINVAL;
> }
>
> @@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>
> /* Header size calculation must not overflow uint32_t */
> if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
> + error_setg(errp, "QED header size is too large");
> return -EINVAL;
> }
>
> @@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
> if ((uint64_t)s->header.backing_filename_offset +
> s->header.backing_filename_size >
> s->header.cluster_size * s->header.header_size) {
> + error_setg(errp, "QED backing filename offset is invalid");
> return -EINVAL;
> }
>
> @@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
> bs->auto_backing_file,
> sizeof(bs->auto_backing_file));
> if (ret < 0) {
> + error_setg(errp, "Failed to read backing filename");
> return ret;
> }
> pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> @@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>
> ret = qed_write_header_sync(s);
> if (ret) {
> + error_setg(errp, "Failed to update header");
> return ret;
> }
>
> @@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>
> ret = qed_read_l1_table_sync(s);
> if (ret) {
> + error_setg(errp, "Failed to read L1 table");
> goto out;
> }
>
> @@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>
> ret = qed_check(s, &result, true);
> if (ret) {
> + error_setg(errp, "Image corrupted");
> goto out;
> }
> }
> @@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
> Error **errp)
> {
> BDRVQEDState *s = bs->opaque;
> - Error *local_err = NULL;
> int ret;
>
> bdrv_qed_close(bs);
>
> bdrv_qed_init_state(bs);
> qemu_co_mutex_lock(&s->table_lock);
> - ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
> + ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
> qemu_co_mutex_unlock(&s->table_lock);
> - if (local_err) {
> - error_propagate_prepend(errp, local_err,
> - "Could not reopen qed layer: ");
> - return;
> - } else if (ret < 0) {
> - error_setg_errno(errp, -ret, "Could not reopen qed layer");
> - return;
> + if (ret < 0) {
> + error_prepend(errp, "Could not reopen qed layer: ");
> }
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-09-18 16:01 ` Vladimir Sementsov-Ogievskiy
@ 2020-09-18 16:10 ` Greg Kurz
0 siblings, 0 replies; 27+ messages in thread
From: Greg Kurz @ 2020-09-18 16:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, Alberto Garcia, pavel.dovgaluk, qemu-block, qemu-devel,
armbru, stefanha, pbonzini, mreitz, jsnow, ari
On Fri, 18 Sep 2020 19:01:34 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 18.09.2020 18:51, Alberto Garcia wrote:
> > On Fri 18 Sep 2020 05:30:06 PM CEST, Greg Kurz wrote:
> >>> qcow2_do_open correctly sets errp on each failure path. So, we can
> >>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> >>> propagation. We should use ERRP_GUARD() (accordingly to comment in
> >>> include/qapi/error.h) together with error_append() call which we add to
> >>> avoid problems with error_fatal.
> >>>
> >>
> >> The wording gives the impression that we add error_append() to avoid problems
> >> with error_fatal which is certainly not true. Also it isn't _append() but
> >> _prepend() :)
> >>
> >> What about ?
> >>
> >> "Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
> >> to avoid problems with the error_prepend() call if errp is
> >> &error_fatal."
>
> OK for me.
>
> >
> > I had to go to the individual error functions to see what "it doesn't
> > work with &error_fatal" actually means.
> >
> > So in a case like qcow2_do_open() which has:
> >
> > error_setg(errp, ...)
> > error_append_hint(errp, ...)
> >
> > As far as I can see this works just fine without ERRP_GUARD() and with
> > error_fatal, the difference is that if we don't use the guard then the
> > process exists during error_setg(), and if we use the guard it exists
> > during the implicit error_propagate() call triggered by its destruction
> > at the end of the function. In this latter case the printed error
> > message would include the hint.
> >
>
> Yes the only problem is that without ERRP_GUARD we lose the hint in case of error_fatal.
>
Yeah, so rather:
"Add ERRP_GUARD() as mandated by the documentation in include/qapi/error.h
so that error_prepend() is actually called even if errp is &error_fatal."
Cheers,
--
Greg
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-09-18 16:25 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-17 19:55 [PATCH v2 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 03/13] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 04/13] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 05/13] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 06/13] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 07/13] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
2020-09-18 15:04 ` Alberto Garcia
2020-09-17 19:55 ` [PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
2020-09-18 14:54 ` Alberto Garcia
2020-09-18 14:59 ` Vladimir Sementsov-Ogievskiy
2020-09-18 15:15 ` Greg Kurz
2020-09-17 19:55 ` [PATCH v2 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
2020-09-18 15:21 ` Alberto Garcia
2020-09-17 19:55 ` [PATCH v2 11/13] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
2020-09-17 19:55 ` [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
2020-09-18 15:26 ` Alberto Garcia
2020-09-18 15:30 ` Greg Kurz
2020-09-18 15:51 ` Alberto Garcia
2020-09-18 16:01 ` Vladimir Sementsov-Ogievskiy
2020-09-18 16:10 ` Greg Kurz
2020-09-17 19:55 ` [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
2020-09-18 16:07 ` Greg Kurz
2020-09-17 20:15 ` [PATCH v2 00/13] block: deal with errp: part I no-reply
2020-09-18 12:27 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.