* [PULL 1/5] migration: dirty-bitmap: Use struct for alias map inner members
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
@ 2021-02-12 23:21 ` Eric Blake
2021-02-12 23:21 ` [PULL 2/5] migration: dirty-bitmap: Allow control of bitmap persistence Eric Blake
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2021-02-12 23:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Krempa, open list:Dirty Bitmaps, Juan Quintela,
Dr. David Alan Gilbert, Vladimir Sementsov-Ogievskiy,
Stefan Hajnoczi, John Snow
From: Peter Krempa <pkrempa@redhat.com>
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 <pkrempa@redhat.com>
Message-Id: <fc5f27e1fe16cb75e08a248c2d938de3997b9bfb.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: adjust long lines]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 c61d382be87c..b39c13ce4ebe 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;
}
}
--
2.30.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 2/5] migration: dirty-bitmap: Allow control of bitmap persistence
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
2021-02-12 23:21 ` [PULL 1/5] migration: dirty-bitmap: Use struct for alias map inner members Eric Blake
@ 2021-02-12 23:21 ` Eric Blake
2021-02-12 23:21 ` [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap Eric Blake
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2021-02-12 23:21 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, Peter Krempa, open list:Dirty Bitmaps, Juan Quintela,
Dr. David Alan Gilbert, Markus Armbruster,
Vladimir Sementsov-Ogievskiy, Stefan Hajnoczi, John Snow
From: Peter Krempa <pkrempa@redhat.com>
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 <pkrempa@redhat.com>
Message-Id: <b20afb675917b86f6359ac3591166ac6d4233573.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: grammar tweaks, drop dead conditional]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
qapi/migration.json | 19 ++++++++++++++++++-
migration/block-dirty-bitmap.c | 29 ++++++++++++++++++++++++++---
2 files changed, 44 insertions(+), 4 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index ce14d78071a5..6e5943fbb443 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'
} }
##
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b39c13ce4ebe..975093610a41 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) {
--
2.30.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
2021-02-12 23:21 ` [PULL 1/5] migration: dirty-bitmap: Use struct for alias map inner members Eric Blake
2021-02-12 23:21 ` [PULL 2/5] migration: dirty-bitmap: Allow control of bitmap persistence Eric Blake
@ 2021-02-12 23:21 ` Eric Blake
2021-02-15 12:31 ` Kevin Wolf
2021-02-12 23:21 ` [PULL 4/5] block: return status from bdrv_append and friends Eric Blake
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2021-02-12 23:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, open list:Block layer core, Max Reitz
From: Peter Krempa <pkrempa@redhat.com>
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 <pkrempa@redhat.com>
Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: Adjust test for explicit read_zeroes=False]
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 43264d883d79..63036f6a6e13 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 cafb8161f7b1..12e9ab7d571b 100644
--- a/tests/qemu-iotests/300.out
+++ b/tests/qemu-iotests/300.out
@@ -1,5 +1,5 @@
-.....................................
+.......................................
----------------------------------------------------------------------
-Ran 37 tests
+Ran 39 tests
OK
--
2.30.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-12 23:21 ` [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap Eric Blake
@ 2021-02-15 12:31 ` Kevin Wolf
2021-02-15 16:46 ` Eric Blake
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-02-15 12:31 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Krempa, qemu-devel, open list:Block layer core, Max Reitz
Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
> From: Peter Krempa <pkrempa@redhat.com>
>
> 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 <pkrempa@redhat.com>
> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> [eblake: Adjust test for explicit read_zeroes=False]
> Signed-off-by: Eric Blake <eblake@redhat.com>
This breaks 297:
--- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
+++ 297.out.bad
@@ -1,2 +1,8 @@
=== pylint ===
+************* Module 300
+300:605:0: C0301: Line too long (80/79) (line-too-long)
+300:677:0: C0301: Line too long (98/79) (line-too-long)
=== mypy ===
+300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
+Found 1 error in 1 file (checked 1 source file)
+
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 12:31 ` Kevin Wolf
@ 2021-02-15 16:46 ` Eric Blake
2021-02-15 17:09 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2021-02-15 16:46 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Krempa, John Snow, qemu-devel, open list:Block layer core,
Max Reitz
On 2/15/21 6:31 AM, Kevin Wolf wrote:
> Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
>> From: Peter Krempa <pkrempa@redhat.com>
>>
>> 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 <pkrempa@redhat.com>
>> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> [eblake: Adjust test for explicit read_zeroes=False]
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> This breaks 297:
>
> --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
> +++ 297.out.bad
> @@ -1,2 +1,8 @@
> === pylint ===
> +************* Module 300
> +300:605:0: C0301: Line too long (80/79) (line-too-long)
> +300:677:0: C0301: Line too long (98/79) (line-too-long)
These two are easy fixes (add line breaks for shorter lines), but this:
> === mypy ===
> +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
> +Found 1 error in 1 file (checked 1 source file)
is beyond my skill. The typing at line 33:
BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
is insufficient to allow our new 'transform' member in the new
transform_mapping() -> Block BitmapMapping near line 677:
'bitmaps': [
{
'name': 'bmap-a',
'alias': 'bmap-a',
'transform':
{
'persistent': True
}
},
but I'm not sure how to tell python the right type it should be. John?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 16:46 ` Eric Blake
@ 2021-02-15 17:09 ` Kevin Wolf
2021-02-15 18:25 ` Eric Blake
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2021-02-15 17:09 UTC (permalink / raw)
To: Eric Blake
Cc: Peter Krempa, John Snow, qemu-devel, open list:Block layer core,
Max Reitz
Am 15.02.2021 um 17:46 hat Eric Blake geschrieben:
> On 2/15/21 6:31 AM, Kevin Wolf wrote:
> > Am 13.02.2021 um 00:21 hat Eric Blake geschrieben:
> >> From: Peter Krempa <pkrempa@redhat.com>
> >>
> >> 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 <pkrempa@redhat.com>
> >> Message-Id: <d9c8e9827e9b6001b2dd1b92e64aab858e6d2a86.1613150869.git.pkrempa@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> [eblake: Adjust test for explicit read_zeroes=False]
> >> Signed-off-by: Eric Blake <eblake@redhat.com>
> >
> > This breaks 297:
> >
> > --- /home/kwolf/source/qemu/tests/qemu-iotests/297.out
> > +++ 297.out.bad
> > @@ -1,2 +1,8 @@
> > === pylint ===
> > +************* Module 300
> > +300:605:0: C0301: Line too long (80/79) (line-too-long)
> > +300:677:0: C0301: Line too long (98/79) (line-too-long)
>
> These two are easy fixes (add line breaks for shorter lines), but this:
>
> > === mypy ===
> > +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
> > +Found 1 error in 1 file (checked 1 source file)
>
> is beyond my skill. The typing at line 33:
>
> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>
> is insufficient to allow our new 'transform' member in the new
> transform_mapping() -> Block BitmapMapping near line 677:
>
> 'bitmaps': [
> {
> 'name': 'bmap-a',
> 'alias': 'bmap-a',
> 'transform':
> {
> 'persistent': True
> }
> },
>
> but I'm not sure how to tell python the right type it should be. John?
To be honest, this looks sufficiently like JSON that I would just go for
List[Dict[str, Any]] (as long as recursive types don't work), but if you
really want to have an explicit type, I think you'd have to replace the
rightmost str with Union[str, Dict[str, bool]] to allow both.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 17:09 ` Kevin Wolf
@ 2021-02-15 18:25 ` Eric Blake
2021-02-15 19:00 ` John Snow
0 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2021-02-15 18:25 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Krempa, John Snow, qemu-devel, open list:Block layer core,
Max Reitz
On 2/15/21 11:09 AM, Kevin Wolf wrote:
>>> This breaks 297:
>>> === mypy ===
>>> +300:646: error: Dict entry 2 has incompatible type "str": "Dict[str, bool]"; expected "str": "str"
>>> +Found 1 error in 1 file (checked 1 source file)
>>
>> is beyond my skill. The typing at line 33:
>>
>> BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>>
>> is insufficient to allow our new 'transform' member in the new
>> transform_mapping() -> Block BitmapMapping near line 677:
>>
>> 'bitmaps': [
>> {
>> 'name': 'bmap-a',
>> 'alias': 'bmap-a',
>> 'transform':
>> {
>> 'persistent': True
>> }
>> },
>>
>> but I'm not sure how to tell python the right type it should be. John?
>
> To be honest, this looks sufficiently like JSON that I would just go for
> List[Dict[str, Any]] (as long as recursive types don't work), but if you
> really want to have an explicit type, I think you'd have to replace the
> rightmost str with Union[str, Dict[str, bool]] to allow both.
Indeed, I played with it before reading your response, and came up with
this. Shall I turn it into a formal patch?
diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
index 63036f6a6e13..7501bd1018e2 100755
--- i/tests/qemu-iotests/300
+++ w/tests/qemu-iotests/300
@@ -30,7 +30,10 @@ import iotests
# pylint: disable=wrong-import-order
import qemu
-BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
+BlockBitmapMapping = List[Dict[str,
+ Union[str,
+ List[Dict[str,
+ Union[str, Dict[str,
bool]]]]]]]
mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
@@ -602,7 +605,8 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):
class TestAliasTransformMigration(TestDirtyBitmapMigration):
"""
- Tests the 'transform' option which modifies bitmap persistence on
migration.
+ Tests the 'transform' option which modifies bitmap persistence on
+ migration.
"""
src_node_name = 'node-a'
@@ -674,7 +678,8 @@ class
TestAliasTransformMigration(TestDirtyBitmapMigration):
bitmaps = self.vm_b.query_bitmaps()
for node in bitmaps:
- bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])
for bmap in bitmaps[node]))
+ bitmaps[node] = sorted(((bmap['name'], bmap['persistent'])
+ for bmap in bitmaps[node]))
self.assertEqual(bitmaps,
{'node-a': [('bmap-a', True), ('bmap-b', False)],
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 18:25 ` Eric Blake
@ 2021-02-15 19:00 ` John Snow
2021-02-15 20:26 ` Eric Blake
0 siblings, 1 reply; 14+ messages in thread
From: John Snow @ 2021-02-15 19:00 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf
Cc: Peter Krempa, qemu-devel, open list:Block layer core, Max Reitz
On 2/15/21 1:25 PM, Eric Blake wrote:
> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
> +BlockBitmapMapping = List[Dict[str,
> + Union[str,
> + List[Dict[str,
> + Union[str, Dict[str,
> bool]]]]]]]
That looks *very* beefy.
Is the Union because that union is valid for every key, or because every
key has a potentially different value that is specific to that key?
if it's the latter, I'd ditch the Union and just go with:
Dict[str, object], or
Dict[str, Any]
object: will allow any type, but keeps strict checking enabled. If you
try to use that value later on without a cast, mypy will warn you if you
are using it in a manner not guaranteed by the "object" type. Can be
useful if you are passing values to a function that already does RTTI to
determine behavior.
Any: Also allows any type, but enables gradual typing. If you later
"assume" the type of this value, mypy will say nothing. Can be useful
when you've just got a job to do and the right tool would have been a
recursive type or a TypedDict (unavailable in Python 3.6.)
--js
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 19:00 ` John Snow
@ 2021-02-15 20:26 ` Eric Blake
2021-02-15 21:37 ` John Snow
0 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2021-02-15 20:26 UTC (permalink / raw)
To: John Snow, Kevin Wolf
Cc: Peter Krempa, qemu-devel, open list:Block layer core, Max Reitz
On 2/15/21 1:00 PM, John Snow wrote:
> On 2/15/21 1:25 PM, Eric Blake wrote:
>> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>> +BlockBitmapMapping = List[Dict[str,
>> + Union[str,
>> + List[Dict[str,
>> + Union[str, Dict[str,
>> bool]]]]]]]
>
> That looks *very* beefy.
>
> Is the Union because that union is valid for every key, or because every
> key has a potentially different value that is specific to that key?
>
> if it's the latter, I'd ditch the Union and just go with:
>
> Dict[str, object], or
> Dict[str, Any]
>
> object: will allow any type, but keeps strict checking enabled. If you
> try to use that value later on without a cast, mypy will warn you if you
> are using it in a manner not guaranteed by the "object" type. Can be
> useful if you are passing values to a function that already does RTTI to
> determine behavior.
We're in luck; both 297 and 300 still pass with this applied on top of
my previous attempt:
diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
index 7501bd1018e2..adb927629747 100755
--- i/tests/qemu-iotests/300
+++ w/tests/qemu-iotests/300
@@ -22,7 +22,7 @@
import os
import random
import re
-from typing import Dict, List, Optional, Union
+from typing import Dict, List, Optional
import iotests
@@ -30,10 +30,7 @@ import iotests
# pylint: disable=wrong-import-order
import qemu
-BlockBitmapMapping = List[Dict[str,
- Union[str,
- List[Dict[str,
- Union[str, Dict[str,
bool]]]]]]]
+BlockBitmapMapping = List[Dict[str, object]]
mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
>
> Any: Also allows any type, but enables gradual typing. If you later
> "assume" the type of this value, mypy will say nothing. Can be useful
> when you've just got a job to do and the right tool would have been a
> recursive type or a TypedDict (unavailable in Python 3.6.)
I'm not too worried about needing to further enhance the type-checking
on an individual iotest.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap
2021-02-15 20:26 ` Eric Blake
@ 2021-02-15 21:37 ` John Snow
0 siblings, 0 replies; 14+ messages in thread
From: John Snow @ 2021-02-15 21:37 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf
Cc: Peter Krempa, qemu-devel, open list:Block layer core, Max Reitz
On 2/15/21 3:26 PM, Eric Blake wrote:
> On 2/15/21 1:00 PM, John Snow wrote:
>> On 2/15/21 1:25 PM, Eric Blake wrote:
>>> -BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]]]]]
>>> +BlockBitmapMapping = List[Dict[str,
>>> + Union[str,
>>> + List[Dict[str,
>>> + Union[str, Dict[str,
>>> bool]]]]]]]
>>
>> That looks *very* beefy.
>>
>> Is the Union because that union is valid for every key, or because every
>> key has a potentially different value that is specific to that key?
>>
>> if it's the latter, I'd ditch the Union and just go with:
>>
>> Dict[str, object], or
>> Dict[str, Any]
>>
>> object: will allow any type, but keeps strict checking enabled. If you
>> try to use that value later on without a cast, mypy will warn you if you
>> are using it in a manner not guaranteed by the "object" type. Can be
>> useful if you are passing values to a function that already does RTTI to
>> determine behavior.
>
> We're in luck; both 297 and 300 still pass with this applied on top of
> my previous attempt:
>
> diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
> index 7501bd1018e2..adb927629747 100755
> --- i/tests/qemu-iotests/300
> +++ w/tests/qemu-iotests/300
> @@ -22,7 +22,7 @@
> import os
> import random
> import re
> -from typing import Dict, List, Optional, Union
> +from typing import Dict, List, Optional
>
> import iotests
>
> @@ -30,10 +30,7 @@ import iotests
> # pylint: disable=wrong-import-order
> import qemu
>
> -BlockBitmapMapping = List[Dict[str,
> - Union[str,
> - List[Dict[str,
> - Union[str, Dict[str,
> bool]]]]]]]
> +BlockBitmapMapping = List[Dict[str, object]]
>
nice :)
> mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
>
>
>
>>
>> Any: Also allows any type, but enables gradual typing. If you later
>> "assume" the type of this value, mypy will say nothing. Can be useful
>> when you've just got a job to do and the right tool would have been a
>> recursive type or a TypedDict (unavailable in Python 3.6.)
>
> I'm not too worried about needing to further enhance the type-checking
> on an individual iotest.
>
Yes, Agreed. I have been going very "overboard" with the python and QAPI
types, but I consider those libraries that might have need of such
pedantic types.
The iotests themselves? eh.
Just figured I'd give you a range of options to choose from and you'd
pick the best one.
--js
PS: I really really really really really wish that 3.6 had TypedDict.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PULL 4/5] block: return status from bdrv_append and friends
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
` (2 preceding siblings ...)
2021-02-12 23:21 ` [PULL 3/5] qemu-iotests: 300: Add test case for modifying persistence of bitmap Eric Blake
@ 2021-02-12 23:21 ` Eric Blake
2021-02-12 23:21 ` [PULL 5/5] block: use return status of bdrv_append() Eric Blake
2021-02-14 18:45 ` [PULL 0/5] bitmaps patches through 2021-02-12 Peter Maydell
5 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2021-02-12 23:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Alberto Garcia,
open list:Block layer core, Max Reitz
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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 <vsementsov@virtuozzo.com>
Message-Id: <20210202124956.63146-2-vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/block/block.h | 12 ++++-----
block.c | 58 +++++++++++++++++++++++++++----------------
2 files changed, 43 insertions(+), 27 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 0a9f2c187cdb..2c235a29e822 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,
diff --git a/block.c b/block.c
index 4e52b1c588cd..fdb0261cb324 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)
--
2.30.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PULL 5/5] block: use return status of bdrv_append()
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
` (3 preceding siblings ...)
2021-02-12 23:21 ` [PULL 4/5] block: return status from bdrv_append and friends Eric Blake
@ 2021-02-12 23:21 ` Eric Blake
2021-02-14 18:45 ` [PULL 0/5] bitmaps patches through 2021-02-12 Peter Maydell
5 siblings, 0 replies; 14+ messages in thread
From: Eric Blake @ 2021-02-12 23:21 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy, Alberto Garcia,
open list:Block layer core, Markus Armbruster, Max Reitz,
John Snow
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Now bdrv_append returns status and we can drop all the local_err things
around it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Message-Id: <20210202124956.63146-3-vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
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 fdb0261cb324..c682c3e3b96b 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 6e7e7bf34047..d1253e1aa63f 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 71db7ba7472e..dd9ba87349e3 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 8e1ad6eceb57..fad270193888 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 b250b9b95957..cd438e60e35a 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 8cff13830e3e..c4f7d1603924 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);
}
--
2.30.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PULL 0/5] bitmaps patches through 2021-02-12
2021-02-12 23:21 [PULL 0/5] bitmaps patches through 2021-02-12 Eric Blake
` (4 preceding siblings ...)
2021-02-12 23:21 ` [PULL 5/5] block: use return status of bdrv_append() Eric Blake
@ 2021-02-14 18:45 ` Peter Maydell
5 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2021-02-14 18:45 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers
On Fri, 12 Feb 2021 at 23:24, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit eac92d316351b855ba79eb374dd21cc367f1f9c1:
>
> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20210211-1' into staging (2021-02-11 19:57:50 +0000)
>
> are available in the Git repository at:
>
> https://repo.or.cz/qemu/ericb.git tags/pull-bitmaps-2021-02-12
>
> for you to fetch changes up to 934aee14d36e67468260635af61c387227cdaf78:
>
> block: use return status of bdrv_append() (2021-02-12 15:39:44 -0600)
>
> ----------------------------------------------------------------
> bitmaps patches for 2021-02-12
>
> - add 'transform' member to manipulate bitmaps across migration
> - work towards better error handling during bdrv_open
>
> ----------------------------------------------------------------
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 14+ messages in thread