| Message ID | 20201010110730.1575180-1-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/block-dirty-bitmap: fix uninitialized variable warning | expand |
Chen Qun <kuhn.chenqun@huawei.com> 于2020年10月10日周六 下午7:08写道： > > 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.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> Reviewed-by: Li Qiang <liq3ea@gmail.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); > > -- > 2.23.0 > >
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.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@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". Thanks, Laurent
Laurent Vivier <laurent@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.robot@huawei.com> > > Signed-off-by: Chen Qun <kuhn.chenqun@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. Thanks, Li Qiang > Thanks, > Laurent >
Le 13/10/2020 à 03:34, Li Qiang a écrit : > Laurent Vivier <laurent@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.robot@huawei.com> >>> Signed-off-by: Chen Qun <kuhn.chenqun@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. Thanks, Laurent
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, Yes, you're right. It will be modified value when cancel_incoming_locked is executed. Thanks. > So I think this can simply assign 'bitmap_name' to NULL to make > compiler happy. > Hi Qiang, I think your suggestion is feasible. But can we just give it a default value when defining variables? I think the default value could be "bitmap_name = s->bitmap_alias", and then we can delete the else statement above. Thanks, Chen Qun
> -----Original Message----- > From: Laurent Vivier [mailto:laurent@vivier.eu] > Sent: Tuesday, October 13, 2020 3:11 PM > To: Li Qiang <liq3ea@gmail.com> > Cc: Fam Zheng <fam@euphon.net>; ganqixin <ganqixin@huawei.com>; > vsementsov@virtuozzo.com; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; qemu-block@nongnu.org; Juan Quintela > <quintela@redhat.com>; qemu-trivial@nongnu.org; Qemu Developers > <qemu-devel@nongnu.org>; Dr. David Alan Gilbert <dgilbert@redhat.com>; > Stefan Hajnoczi <stefanha@redhat.com>; Euler Robot > <euler.robot@huawei.com>; Chenqun (kuhn) <kuhn.chenqun@huawei.com>; > Max Reitz <mreitz@redhat.com>; John Snow <jsnow@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 <laurent@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.robot@huawei.com> > >>> Signed-off-by: Chen Qun <kuhn.chenqun@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) {
13.10.2020 10:49, Chenqun (kuhn) wrote: >> -----Original Message----- >> From: Laurent Vivier [mailto:laurent@vivier.eu] >> Sent: Tuesday, October 13, 2020 3:11 PM >> To: Li Qiang <liq3ea@gmail.com> >> Cc: Fam Zheng <fam@euphon.net>; ganqixin <ganqixin@huawei.com>; >> vsementsov@virtuozzo.com; Zhanghailiang >> <zhang.zhanghailiang@huawei.com>; qemu-block@nongnu.org; Juan Quintela >> <quintela@redhat.com>; qemu-trivial@nongnu.org; Qemu Developers >> <qemu-devel@nongnu.org>; Dr. David Alan Gilbert <dgilbert@redhat.com>; >> Stefan Hajnoczi <stefanha@redhat.com>; Euler Robot >> <euler.robot@huawei.com>; Chenqun (kuhn) <kuhn.chenqun@huawei.com>; >> Max Reitz <mreitz@redhat.com>; John Snow <jsnow@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 <laurent@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.robot@huawei.com> >>>>> Signed-off-by: Chen Qun <kuhn.chenqun@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.
> >>>> 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.robot@huawei.com> > >>>>> Signed-off-by: Chen Qun <kuhn.chenqun@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. > OK, I'll update it later. Thanks, Chen Qun
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);
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.robot@huawei.com> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> --- migration/block-dirty-bitmap.c | 2 -- 1 file changed, 2 deletions(-)