13.10.2020 10:49, Chenqun (kuhn) wrote:
-----Original Message-----
From: Laurent Vivier [mailto:laur...@vivier.eu]
Sent: Tuesday, October 13, 2020 3:11 PM
To: Li Qiang <liq...@gmail.com>
Cc: Fam Zheng <f...@euphon.net>; ganqixin <ganqi...@huawei.com>;
vsement...@virtuozzo.com; Zhanghailiang
<zhang.zhanghaili...@huawei.com>; qemu-bl...@nongnu.org; Juan Quintela
<quint...@redhat.com>; qemu-triv...@nongnu.org; Qemu Developers
<qemu-devel@nongnu.org>; Dr. David Alan Gilbert <dgilb...@redhat.com>;
Stefan Hajnoczi <stefa...@redhat.com>; Euler Robot
<euler.ro...@huawei.com>; Chenqun (kuhn) <kuhn.chen...@huawei.com>;
Max Reitz <mre...@redhat.com>; John Snow <js...@redhat.com>
Subject: Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable
warning
Le 13/10/2020 à 03:34, Li Qiang a écrit :
Laurent Vivier <laur...@vivier.eu> 于2020年10月12日周一 下午11:33
写道：
Le 10/10/2020 à 13:07, Chen Qun a écrit :
This if statement judgment is redundant and it will cause a warning:
migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ may
be used uninitialized in this function [-Wmaybe-uninitialized]
g_strlcpy(s->bitmap_name, bitmap_name,
sizeof(s->bitmap_name));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Reported-by: Euler Robot <euler.ro...@huawei.com>
Signed-off-by: Chen Qun <kuhn.chen...@huawei.com>
---
migration/block-dirty-bitmap.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/migration/block-dirty-bitmap.c
b/migration/block-dirty-bitmap.c index 5bef793ac0..e09ea4f22b 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1084,9 +1084,7 @@ static int dirty_bitmap_load_header(QEMUFile
*f, DBMLoadState *s,
} else {
bitmap_name = s->bitmap_alias;
}
- }
- if (!s->cancelled) {
g_strlcpy(s->bitmap_name, bitmap_name,
sizeof(s->bitmap_name));
s->bitmap = bdrv_find_dirty_bitmap(s->bs,
s->bitmap_name);
I don't think it's correct as "cancel_incoming_locked(s)" can change
the value of "s->cancelled".
Hi Laurent,
You're right. So I think this can simply assign 'bitmap_name' to NULL
to make compiler happy.
Yes, and adding a comment before the second "if (!s->cancelled) {" to explain
the value can be changed by "cancel_incoming_locked(s)" would avoid to have
this kind of patch posted regularly to the ML.
Hi Laurent,
We give the bitmap_name a default value (s->bitmap_alias) so that we can remove
the assignment of the else branch statement.
And then we merge the two if statements("if (!s->cancelled)", "if (bitmap_alias_map)") ,
avoids confusion the two "if (!s->cancelled)".
Thanks,
Chen Qun
The code show as that:
@@ -1064,15 +1064,14 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
assert(nothing || s->cancelled || !!alias_map == !!bitmap_alias_map);
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
- const char *bitmap_name;
+ const char *bitmap_name = s->bitmap_alias;
if (!qemu_get_counted_string(f, s->bitmap_alias)) {
error_report("Unable to read bitmap alias string");
return -EINVAL;
}
- if (!s->cancelled) {
- if (bitmap_alias_map) {
+ if (!s->cancelled && bitmap_alias_map) {
bitmap_name = g_hash_table_lookup(bitmap_alias_map,
s->bitmap_alias);
if (!bitmap_name) {
@@ -1081,9 +1080,6 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
s->bs->node_name, s->node_alias);
cancel_incoming_locked(s);
}
- } else {
- bitmap_name = s->bitmap_alias;
- }
}
if (!s->cancelled) {
Sounds good.
--
Best regards,
Vladimir
