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 07:49:15 +0000

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

reply via email to

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