Notice that we changed the test of ->has_block_bitmap_mapping
for the test that block_bitmap_mapping is not NULL.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
Make it return const (vladimir)
---
migration/block-dirty-bitmap.c | 14 ++++++++------
migration/options.c | 7 +++++++
migration/options.h | 1 +
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index a6ffae0002..6624f39bc6 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -605,11 +605,12 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
SaveBitmapState *dbms;
GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
BlockBackend *blk;
- const MigrationParameters *mig_params = &migrate_get_current()->parameters;
GHashTable *alias_map = NULL;
+ const BitmapMigrationNodeAliasList *block_bitmap_mapping =
+ migrate_block_bitmap_mapping();
- if (mig_params->has_block_bitmap_mapping) {
- alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+ if (block_bitmap_mapping) {
+ alias_map = construct_alias_map(block_bitmap_mapping, true,
&error_abort);
}
@@ -1158,7 +1159,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
{
GHashTable *alias_map = NULL;
- const MigrationParameters *mig_params = &migrate_get_current()->parameters;
+ const BitmapMigrationNodeAliasList *block_bitmap_mapping =
+ migrate_block_bitmap_mapping();
DBMLoadState *s = &((DBMState *)opaque)->load;
int ret = 0;
@@ -1170,8 +1172,8 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
return -EINVAL;
}
- if (mig_params->has_block_bitmap_mapping) {
- alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+ if (block_bitmap_mapping) {
+ alias_map = construct_alias_map(block_bitmap_mapping,
false, &error_abort);
}
diff --git a/migration/options.c b/migration/options.c
index 1aa9575bc0..1840a3b220 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -455,6 +455,13 @@ void qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
/* parameters */
+const BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void)
+{
+ MigrationState *s = migrate_get_current();
+
+ return s->parameters.block_bitmap_mapping;
+}
+
bool migrate_block_incremental(void)
{
MigrationState *s = migrate_get_current();
diff --git a/migration/options.h b/migration/options.h
index 819c1b1ce3..9bcb9e558c 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -62,6 +62,7 @@ bool migrate_cap_set(int cap, bool value, Error **errp);
/* parameters */
+const BitmapMigrationNodeAliasList *migrate_block_bitmap_mapping(void);
bool migrate_block_incremental(void);
uint32_t migrate_checkpoint_delay(void);
int migrate_compress_level(void);
--
2.40.0Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: > Notice that we changed the test of ->has_block_bitmap_mapping > for the test that block_bitmap_mapping is not NULL. > > Signed-off-by: Juan Quintela <quintela@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > --- > > Make it return const (vladimir) (I don't think this part was actually meant for the commit message) This commit broke qemu-iotests 300 on master. Please have a look. Kevin
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben:
>> Notice that we changed the test of ->has_block_bitmap_mapping
>> for the test that block_bitmap_mapping is not NULL.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>> ---
>>
>> Make it return const (vladimir)
>
> (I don't think this part was actually meant for the commit message)
yeap. My understandig has always been that this is the way to put
commenst for the email.
> This commit broke qemu-iotests 300 on master. Please have a look.
if (s->parameters.has_block_bitmap_mapping)
return s->parameters.block_bitmap_mapping;
The test has a case where s->parameters.has_block_bitmap_mapping is true
but s->parameters.block_bitmap_mapping is false.
If that combination is right, then we need two functions becase the
asumtion that I did is false.
Vladimir?
Later, Juan.
> Kevin
Am 03.05.2023 um 19:15 hat Juan Quintela geschrieben: > Kevin Wolf <kwolf@redhat.com> wrote: > > Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: > >> Notice that we changed the test of ->has_block_bitmap_mapping > >> for the test that block_bitmap_mapping is not NULL. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > >> > >> --- > >> > >> Make it return const (vladimir) > > > > (I don't think this part was actually meant for the commit message) > > yeap. My understandig has always been that this is the way to put > commenst for the email. Yes, but this only works if you then actually apply the patch from the mail with "git am". Seems you directly cherry-picked your local commit instead, so the comment below "---" has now become part of the git history. We were asked a while ago to always use "git am -m" to include the Message-ID header from the email, so applying from the list is what we should be doing anyway, even for our own patches. Kevin
Kevin Wolf <kwolf@redhat.com> wrote: > Am 03.05.2023 um 19:15 hat Juan Quintela geschrieben: >> Kevin Wolf <kwolf@redhat.com> wrote: >> > Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: >> >> Notice that we changed the test of ->has_block_bitmap_mapping >> >> for the test that block_bitmap_mapping is not NULL. >> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> >> >> --- >> >> >> >> Make it return const (vladimir) >> > >> > (I don't think this part was actually meant for the commit message) >> >> yeap. My understandig has always been that this is the way to put >> commenst for the email. > > Yes, but this only works if you then actually apply the patch from the > mail with "git am". Seems you directly cherry-picked your local commit > instead, so the comment below "---" has now become part of the git > history. Oops. Yeap, I normally rebase my patches on top of upstream. > We were asked a while ago to always use "git am -m" to include the > Message-ID header from the email, so applying from the list is what we > should be doing anyway, even for our own patches. Oops. Will do from now on. Thanks.
On 03.05.23 20:15, Juan Quintela wrote: > Kevin Wolf <kwolf@redhat.com> wrote: >> Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: >>> Notice that we changed the test of ->has_block_bitmap_mapping >>> for the test that block_bitmap_mapping is not NULL. >>> >>> Signed-off-by: Juan Quintela <quintela@redhat.com> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> >>> --- >>> >>> Make it return const (vladimir) >> >> (I don't think this part was actually meant for the commit message) > > yeap. My understandig has always been that this is the way to put > commenst for the email. > >> This commit broke qemu-iotests 300 on master. Please have a look. > > if (s->parameters.has_block_bitmap_mapping) > return s->parameters.block_bitmap_mapping; > > The test has a case where s->parameters.has_block_bitmap_mapping is true > but s->parameters.block_bitmap_mapping is false. > > If that combination is right, then we need two functions becase the > asumtion that I did is false. > > Vladimir? > Ah right. I forget that the field is list and for lists it's OK to be NULL with corresponding has_* = false, as that's the only way to distinguish specified empty list and unspecified list. -- Best regards, Vladimir
Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: >> Notice that we changed the test of ->has_block_bitmap_mapping >> for the test that block_bitmap_mapping is not NULL. >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> --- >> >> Make it return const (vladimir) > > (I don't think this part was actually meant for the commit message) > > This commit broke qemu-iotests 300 on master. Please have a look. > > Kevin grrr selfNack Just wondering, make check don't run this? I run "make check" before I sent the pull request. Later, Juan.
Am 03.05.2023 um 16:53 hat Juan Quintela geschrieben: > Kevin Wolf <kwolf@redhat.com> wrote: > > Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: > >> Notice that we changed the test of ->has_block_bitmap_mapping > >> for the test that block_bitmap_mapping is not NULL. > >> > >> Signed-off-by: Juan Quintela <quintela@redhat.com> > >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > >> > >> --- > >> > >> Make it return const (vladimir) > > > > (I don't think this part was actually meant for the commit message) > > > > This commit broke qemu-iotests 300 on master. Please have a look. > > > > Kevin > > grrr > > selfNack > > Just wondering, make check don't run this? > > I run "make check" before I sent the pull request. "make check" only runs a subset of iotests because it would take too long otherwise (especially in the context of CI - it wasn't me who made this decision). It comes at the cost that sometimes we catch problems only after merging the patch to git master when a block developer first runs the full set of tests. So I wouldn't blame your testing, it's just something that happens, and when it happens we need to look after it. Kevin
Kevin Wolf <kwolf@redhat.com> wrote: > Am 03.05.2023 um 16:53 hat Juan Quintela geschrieben: >> Kevin Wolf <kwolf@redhat.com> wrote: >> > Am 27.04.2023 um 17:22 hat Juan Quintela geschrieben: >> >> Notice that we changed the test of ->has_block_bitmap_mapping >> >> for the test that block_bitmap_mapping is not NULL. >> >> >> >> Signed-off-by: Juan Quintela <quintela@redhat.com> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> >> >> >> --- >> >> >> >> Make it return const (vladimir) >> > >> > (I don't think this part was actually meant for the commit message) >> > >> > This commit broke qemu-iotests 300 on master. Please have a look. >> > >> > Kevin >> >> grrr >> >> selfNack >> >> Just wondering, make check don't run this? >> >> I run "make check" before I sent the pull request. > > "make check" only runs a subset of iotests because it would take too > long otherwise (especially in the context of CI - it wasn't me who made > this decision). It comes at the cost that sometimes we catch problems > only after merging the patch to git master when a block developer first > runs the full set of tests. Ahhh. > So I wouldn't blame your testing, it's just something that happens, and > when it happens we need to look after it. Thanks. I have sent another email. Found why it is failing. But I am not sure what is wrong the test of the change in the code. Later, Juan.
© 2016 - 2026 Red Hat, Inc.