From a1e708fcda5eab10c866a7d6a4fad4f80b69ad15 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:43 +0300 Subject: block: return status from bdrv_append and friends 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_common() has call to bdrv_check_update_perm(), which reports int status, which seems correct to propagate. Signed-off-by: Vladimir Sementsov-Ogievskiy Message-Id: <20210202124956.63146-2-vsementsov@virtuozzo.com> Reviewed-by: Alberto Garcia Signed-off-by: Eric Blake --- block.c | 58 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 21 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index 4e52b1c588..fdb0261cb3 100644 --- a/block.c +++ b/block.c @@ -2827,14 +2827,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, - Error **errp) +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) { @@ -2853,15 +2854,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 = -EPERM; + 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; } /* @@ -4532,9 +4540,9 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) * With auto_skip=false the error is returned if from has a parent which should * not be updated. */ -static void bdrv_replace_node_common(BlockDriverState *from, - BlockDriverState *to, - bool auto_skip, Error **errp) +static int bdrv_replace_node_common(BlockDriverState *from, + BlockDriverState *to, + bool auto_skip, Error **errp) { BdrvChild *c, *next; GSList *list = NULL, *p; @@ -4556,11 +4564,13 @@ static void bdrv_replace_node_common(BlockDriverState *from, if (auto_skip) { continue; } + ret = -EINVAL; error_setg(errp, "Should not change '%s' link to '%s'", c->name, from->node_name); goto out; } if (c->frozen) { + ret = -EPERM; error_setg(errp, "Cannot change '%s' link to '%s'", c->name, from->node_name); goto out; @@ -4591,14 +4601,18 @@ static void bdrv_replace_node_common(BlockDriverState *from, bdrv_set_perm(to); + ret = 0; + out: g_slist_free(list); bdrv_drained_end(from); bdrv_unref(from); + + return ret; } -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp) +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, + Error **errp) { return bdrv_replace_node_common(from, to, true, errp); } @@ -4619,28 +4633,30 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, * 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; } - /* bs_new is now referenced by its new parents, we don't need the - * additional reference any more. */ + ret = 0; + out: + /* + * bs_new is now referenced by its new parents, we don't need the + * additional reference any more. + */ bdrv_unref(bs_new); + + return ret; } static void bdrv_delete(BlockDriverState *bs) -- cgit v1.2.3-55-g7522 From 934aee14d36e67468260635af61c387227cdaf78 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:44 +0300 Subject: block: use return status of bdrv_append() Now bdrv_append returns status and we can drop all the local_err things around it. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Alberto Garcia Message-Id: <20210202124956.63146-3-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- block.c | 6 ++---- block/backup-top.c | 23 +++++++++++------------ block/commit.c | 6 ++---- block/mirror.c | 6 ++---- blockdev.c | 6 +++--- tests/test-bdrv-graph-mod.c | 6 +++--- 6 files changed, 23 insertions(+), 30 deletions(-) (limited to 'block.c') diff --git a/block.c b/block.c index fdb0261cb3..c682c3e3b9 100644 --- a/block.c +++ b/block.c @@ -3118,7 +3118,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 @@ -3165,9 +3164,8 @@ 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); + ret = bdrv_append(bs_snapshot, bs, errp); + if (ret < 0) { bs_snapshot = NULL; goto out; } diff --git a/block/backup-top.c b/block/backup-top.c index 6e7e7bf340..d1253e1aa6 100644 --- a/block/backup-top.c +++ b/block/backup-top.c @@ -191,7 +191,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source, BlockCopyState **bcs, Error **errp) { - Error *local_err = NULL; + ERRP_GUARD(); + int ret; BDRVBackupTopState *state; BlockDriverState *top; bool appended = false; @@ -224,9 +225,9 @@ 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: "); + ret = bdrv_append(top, source, errp); + if (ret < 0) { + error_prepend(errp, "Cannot append backup-top filter: "); goto fail; } appended = true; @@ -236,19 +237,18 @@ 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: "); + ret = bdrv_child_refresh_perms(top, top->backing, errp); + if (ret < 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, perf->use_copy_range, - write_flags, &local_err); - if (local_err) { - error_prepend(&local_err, "Cannot create block-copy-state: "); + write_flags, errp); + if (!state->bcs) { + error_prepend(errp, "Cannot create block-copy-state: "); goto fail; } *bcs = state->bcs; @@ -266,7 +266,6 @@ fail: } bdrv_drained_end(source); - error_propagate(errp, local_err); return NULL; } diff --git a/block/commit.c b/block/commit.c index 71db7ba747..dd9ba87349 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,9 @@ 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) { + ret = bdrv_append(commit_top_bs, top, errp); + if (ret < 0) { commit_top_bs = NULL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/mirror.c b/block/mirror.c index 8e1ad6eceb..fad2701938 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 b250b9b959..cd438e60e3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1432,6 +1432,7 @@ typedef struct ExternalSnapshotState { static void external_snapshot_prepare(BlkActionState *common, Error **errp) { + int ret; int flags = 0; QDict *options = NULL; Error *local_err = NULL; @@ -1591,9 +1592,8 @@ 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); + ret = bdrv_append(state->new_bs, state->old_bs, errp); + if (ret < 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..c4f7d16039 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); + g_assert_cmpint(ret, <, 0); blk_unref(root); } -- cgit v1.2.3-55-g7522