> -----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) {