From 0d1e450c7b3117ee635a00c81d9a92666ebc7ffa Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Fri, 12 Feb 2021 18:34:23 +0100 Subject: [PATCH 1/5] migration: dirty-bitmap: Use struct for alias map inner members Currently the alias mapping hash stores just strings of the target objects internally. In further patches we'll be adding another member which will need to be stored in the map so pass a copy of the whole BitmapMigrationBitmapAlias QAPI struct into the map. Signed-off-by: Peter Krempa Message-Id: Reviewed-by: Eric Blake [eblake: adjust long lines] Signed-off-by: Eric Blake --- migration/block-dirty-bitmap.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index c61d382be8..b39c13ce4e 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -75,6 +75,8 @@ #include "qemu/id.h" #include "qapi/error.h" #include "qapi/qapi-commands-migration.h" +#include "qapi/qapi-visit-migration.h" +#include "qapi/clone-visitor.h" #include "trace.h" #define CHUNK_SIZE (1 << 10) @@ -224,6 +226,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, AliasMapInnerNode *amin; GHashTable *bitmaps_map; const char *node_map_from, *node_map_to; + GDestroyNotify gdn; if (!id_wellformed(bmna->alias)) { error_setg(errp, "The node alias '%s' is not well-formed", @@ -263,8 +266,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, node_map_to = bmna->node_name; } - bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, - g_free, g_free); + gdn = (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias; + bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, + gdn); amin = g_new(AliasMapInnerNode, 1); *amin = (AliasMapInnerNode){ @@ -276,7 +280,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) { const BitmapMigrationBitmapAlias *bmba = bmbal->value; - const char *bmap_map_from, *bmap_map_to; + const char *bmap_map_from; if (strlen(bmba->alias) > UINT8_MAX) { error_setg(errp, @@ -293,7 +297,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, if (name_to_alias) { bmap_map_from = bmba->name; - bmap_map_to = bmba->alias; if (g_hash_table_contains(bitmaps_map, bmba->name)) { error_setg(errp, "The bitmap '%s'/'%s' is mapped twice", @@ -302,7 +305,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, } } else { bmap_map_from = bmba->alias; - bmap_map_to = bmba->name; if (g_hash_table_contains(bitmaps_map, bmba->alias)) { error_setg(errp, "The bitmap alias '%s'/'%s' is used twice", @@ -311,8 +313,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm, } } - g_hash_table_insert(bitmaps_map, - g_strdup(bmap_map_from), g_strdup(bmap_map_to)); + g_hash_table_insert(bitmaps_map, g_strdup(bmap_map_from), + QAPI_CLONE(BitmapMigrationBitmapAlias, bmba)); } } @@ -538,11 +540,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs, } if (bitmap_aliases) { - bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name); - if (!bitmap_alias) { + BitmapMigrationBitmapAlias *bmap_inner; + + bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name); + if (!bmap_inner) { /* Skip bitmaps with no alias */ continue; } + + bitmap_alias = bmap_inner->alias; } else { if (strlen(bitmap_name) > UINT8_MAX) { error_report("Cannot migrate bitmap '%s' on node '%s': " @@ -1074,13 +1080,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, bitmap_name = s->bitmap_alias; if (!s->cancelled && bitmap_alias_map) { - bitmap_name = g_hash_table_lookup(bitmap_alias_map, - s->bitmap_alias); - if (!bitmap_name) { + BitmapMigrationBitmapAlias *bmap_inner; + + bmap_inner = g_hash_table_lookup(bitmap_alias_map, s->bitmap_alias); + if (!bmap_inner) { error_report("Error: Unknown bitmap alias '%s' on node " "'%s' (alias '%s')", s->bitmap_alias, s->bs->node_name, s->node_alias); cancel_incoming_locked(s); + } else { + bitmap_name = bmap_inner->name; } } From 6e9f21a2aa8a78bc9a512a836a40c79fe50dd2b4 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Fri, 12 Feb 2021 18:34:24 +0100 Subject: [PATCH 2/5] migration: dirty-bitmap: Allow control of bitmap persistence Bitmap's source persistence is transported over the migration stream and the destination mirrors it. In some cases the destination might want to persist bitmaps which are not persistent on the source (e.g. the result of merging bitmaps from a number of layers on the source when migrating into a squashed image) but currently it would need to create another set of persistent bitmaps and merge them. This patch adds a 'transform' property to the alias map which allows overriding the persistence of migrated bitmaps both on the source and destination sides. Signed-off-by: Peter Krempa Message-Id: Reviewed-by: Eric Blake [eblake: grammar tweaks, drop dead conditional] Signed-off-by: Eric Blake --- migration/block-dirty-bitmap.c | 29 ++++++++++++++++++++++++++--- qapi/migration.json | 19 ++++++++++++++++++- 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index b39c13ce4e..975093610a 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -150,6 +150,7 @@ typedef struct DBMLoadState { BdrvDirtyBitmap *bitmap; bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */ + BitmapMigrationBitmapAlias *bmap_inner; /* * cancelled @@ -529,6 +530,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs, } FOR_EACH_DIRTY_BITMAP(bs, bitmap) { + BitmapMigrationBitmapAliasTransform *bitmap_transform = NULL; bitmap_name = bdrv_dirty_bitmap_name(bitmap); if (!bitmap_name) { continue; @@ -549,6 +551,9 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs, } bitmap_alias = bmap_inner->alias; + if (bmap_inner->has_transform) { + bitmap_transform = bmap_inner->transform; + } } else { if (strlen(bitmap_name) > UINT8_MAX) { error_report("Cannot migrate bitmap '%s' on node '%s': " @@ -574,8 +579,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs, if (bdrv_dirty_bitmap_enabled(bitmap)) { dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED; } - if (bdrv_dirty_bitmap_get_persistence(bitmap)) { - dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; + if (bitmap_transform && + bitmap_transform->has_persistent) { + if (bitmap_transform->persistent) { + dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; + } + } else { + if (bdrv_dirty_bitmap_get_persistence(bitmap)) { + dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; + } } QSIMPLEQ_INSERT_TAIL(&s->dbms_list, dbms, entry); @@ -783,6 +795,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) uint32_t granularity = qemu_get_be32(f); uint8_t flags = qemu_get_byte(f); LoadBitmapState *b; + bool persistent; if (s->cancelled) { return 0; @@ -807,7 +820,15 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s) return -EINVAL; } - if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) { + if (s->bmap_inner && + s->bmap_inner->has_transform && + s->bmap_inner->transform->has_persistent) { + persistent = s->bmap_inner->transform->persistent; + } else { + persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT; + } + + if (persistent) { bdrv_dirty_bitmap_set_persistence(s->bitmap, true); } @@ -1091,6 +1112,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s, } else { bitmap_name = bmap_inner->name; } + + s->bmap_inner = bmap_inner; } if (!s->cancelled) { diff --git a/qapi/migration.json b/qapi/migration.json index ce14d78071..6e5943fbb4 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -536,6 +536,19 @@ 'data': [ 'none', 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] } +## +# @BitmapMigrationBitmapAliasTransform: +# +# @persistent: If present, the bitmap will be made persistent +# or transient depending on this parameter. +# +# Since: 6.0 +## +{ 'struct': 'BitmapMigrationBitmapAliasTransform', + 'data': { + '*persistent': 'bool' + } } + ## # @BitmapMigrationBitmapAlias: # @@ -544,12 +557,16 @@ # @alias: An alias name for migration (for example the bitmap name on # the opposite site). # +# @transform: Allows the modification of the migrated bitmap. +# (since 6.0) +# # Since: 5.2 ## { 'struct': 'BitmapMigrationBitmapAlias', 'data': { 'name': 'str', - 'alias': 'str' + 'alias': 'str', + '*transform': 'BitmapMigrationBitmapAliasTransform' } } ## From ca4bfec41d56a1154da89b105048b3462361d0f0 Mon Sep 17 00:00:00 2001 From: Peter Krempa Date: Fri, 12 Feb 2021 18:34:25 +0100 Subject: [PATCH 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap Verify that the modification of the bitmap persistence over migration which is controlled via BitmapMigrationBitmapAliasTransform works properly. Based on TestCrossAliasMigration Signed-off-by: Peter Krempa Message-Id: Reviewed-by: Eric Blake [eblake: Adjust test for explicit read_zeroes=False] Signed-off-by: Eric Blake --- tests/qemu-iotests/300 | 93 ++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/300.out | 4 +- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300 index 43264d883d..63036f6a6e 100755 --- a/tests/qemu-iotests/300 +++ b/tests/qemu-iotests/300 @@ -600,6 +600,99 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration): self.verify_dest_has_all_bitmaps() self.verify_dest_error(None) +class TestAliasTransformMigration(TestDirtyBitmapMigration): + """ + Tests the 'transform' option which modifies bitmap persistence on migration. + """ + + src_node_name = 'node-a' + dst_node_name = 'node-b' + src_bmap_name = 'bmap-a' + dst_bmap_name = 'bmap-b' + + def setUp(self) -> None: + TestDirtyBitmapMigration.setUp(self) + + # Now create another block device and let both have two bitmaps each + result = self.vm_a.qmp('blockdev-add', + node_name='node-b', driver='null-co', + read_zeroes=False) + self.assert_qmp(result, 'return', {}) + + result = self.vm_b.qmp('blockdev-add', + node_name='node-a', driver='null-co', + read_zeroes=False) + self.assert_qmp(result, 'return', {}) + + bmaps_to_add = (('node-a', 'bmap-b'), + ('node-b', 'bmap-a'), + ('node-b', 'bmap-b')) + + for (node, bmap) in bmaps_to_add: + result = self.vm_a.qmp('block-dirty-bitmap-add', + node=node, name=bmap) + self.assert_qmp(result, 'return', {}) + + @staticmethod + def transform_mapping() -> BlockBitmapMapping: + return [ + { + 'node-name': 'node-a', + 'alias': 'node-a', + 'bitmaps': [ + { + 'name': 'bmap-a', + 'alias': 'bmap-a', + 'transform': + { + 'persistent': True + } + }, + { + 'name': 'bmap-b', + 'alias': 'bmap-b' + } + ] + }, + { + 'node-name': 'node-b', + 'alias': 'node-b', + 'bitmaps': [ + { + 'name': 'bmap-a', + 'alias': 'bmap-a' + }, + { + 'name': 'bmap-b', + 'alias': 'bmap-b' + } + ] + } + ] + + def verify_dest_bitmap_state(self) -> None: + bitmaps = self.vm_b.query_bitmaps() + + for node in bitmaps: + bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node])) + + self.assertEqual(bitmaps, + {'node-a': [('bmap-a', True), ('bmap-b', False)], + 'node-b': [('bmap-a', False), ('bmap-b', False)]}) + + def test_transform_on_src(self) -> None: + self.set_mapping(self.vm_a, self.transform_mapping()) + + self.migrate() + self.verify_dest_bitmap_state() + self.verify_dest_error(None) + + def test_transform_on_dst(self) -> None: + self.set_mapping(self.vm_b, self.transform_mapping()) + + self.migrate() + self.verify_dest_bitmap_state() + self.verify_dest_error(None) if __name__ == '__main__': iotests.main(supported_protocols=['file']) diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out index cafb8161f7..12e9ab7d57 100644 --- a/tests/qemu-iotests/300.out +++ b/tests/qemu-iotests/300.out @@ -1,5 +1,5 @@ -..................................... +....................................... ---------------------------------------------------------------------- -Ran 37 tests +Ran 39 tests OK From a1e708fcda5eab10c866a7d6a4fad4f80b69ad15 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:43 +0300 Subject: [PATCH 4/5] 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 +++++++++++++++++++++++++++---------------- include/block/block.h | 12 ++++----- 2 files changed, 43 insertions(+), 27 deletions(-) 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) diff --git a/include/block/block.h b/include/block/block.h index 0a9f2c187c..2c235a29e8 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -356,10 +356,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); BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options, int flags, Error **errp); @@ -373,8 +373,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, From 934aee14d36e67468260635af61c387227cdaf78 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Tue, 2 Feb 2021 15:49:44 +0300 Subject: [PATCH 5/5] 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(-) 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); }