qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable war


From: Chenqun (kuhn)
Subject: RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
Date: Tue, 13 Oct 2020 11:49:32 +0000

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]