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: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
Date: Tue, 13 Oct 2020 14:32:35 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.3.2

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.

--
Best regards,
Vladimir



reply via email to

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