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

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
@ 2020-10-10 11:07 Chen Qun
  2020-10-11  1:55 ` Li Qiang
  2020-10-12 15:31 ` Laurent Vivier
  0 siblings, 2 replies; 9+ messages in thread
From: Chen Qun @ 2020-10-10 11:07 UTC (permalink / raw)
  To: qemu-devel, qemu-trivial
  Cc: fam, ganqixin, vsementsov, zhang.zhanghailiang, qemu-block,
	quintela, dgilbert, mreitz, stefanha, Euler Robot, Chen Qun,
	jsnow

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);
 
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-10 11:07 [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning Chen Qun
@ 2020-10-11  1:55 ` Li Qiang
  2020-10-12 15:31 ` Laurent Vivier
  1 sibling, 0 replies; 9+ messages in thread
From: Li Qiang @ 2020-10-11  1:55 UTC (permalink / raw)
  To: Chen Qun
  Cc: Fam Zheng, vsementsov, zhanghailiang, qemu-block, Juan Quintela,
	qemu-trivial, Stefan Hajnoczi, Qemu Developers,
	Dr. David Alan Gilbert, ganqixin, Euler Robot, Max Reitz,
	John Snow

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-10 11:07 [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning Chen Qun
  2020-10-11  1:55 ` Li Qiang
@ 2020-10-12 15:31 ` Laurent Vivier
  2020-10-13  1:34   ` Li Qiang
  1 sibling, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-10-12 15:31 UTC (permalink / raw)
  To: Chen Qun, qemu-devel, qemu-trivial
  Cc: fam, vsementsov, zhang.zhanghailiang, qemu-block, quintela, jsnow,
	stefanha, dgilbert, mreitz, ganqixin, Euler Robot

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-12 15:31 ` Laurent Vivier
@ 2020-10-13  1:34   ` Li Qiang
  2020-10-13  7:11     ` Laurent Vivier
  2020-10-13  7:27     ` Chenqun (kuhn)
  0 siblings, 2 replies; 9+ messages in thread
From: Li Qiang @ 2020-10-13  1:34 UTC (permalink / raw)
  To: Laurent Vivier
  Cc: Fam Zheng, ganqixin, vsementsov, zhanghailiang, qemu-block,
	Juan Quintela, qemu-trivial, Qemu Developers,
	Dr. David Alan Gilbert, Stefan Hajnoczi, Euler Robot, Chen Qun,
	Max Reitz, John Snow

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
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-13  1:34   ` Li Qiang
@ 2020-10-13  7:11     ` Laurent Vivier
  2020-10-13  7:49       ` Chenqun (kuhn)
  2020-10-13  7:27     ` Chenqun (kuhn)
  1 sibling, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2020-10-13  7:11 UTC (permalink / raw)
  To: Li Qiang
  Cc: Fam Zheng, vsementsov, zhanghailiang, qemu-block, Juan Quintela,
	qemu-trivial, Stefan Hajnoczi, Qemu Developers,
	Dr. David Alan Gilbert, ganqixin, Euler Robot, Chen Qun,
	Max Reitz, John Snow

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-13  7:11     ` Laurent Vivier
@ 2020-10-13  7:49       ` Chenqun (kuhn)
  2020-10-13 11:32         ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Chenqun (kuhn) @ 2020-10-13  7:49 UTC (permalink / raw)
  To: Laurent Vivier, Li Qiang
  Cc: Fam Zheng, vsementsov@virtuozzo.com, Zhanghailiang,
	qemu-block@nongnu.org, Juan Quintela, qemu-trivial@nongnu.org,
	Stefan Hajnoczi, Qemu Developers, Dr. David Alan Gilbert,
	ganqixin, Euler Robot, Max Reitz, John Snow

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-13  7:49       ` Chenqun (kuhn)
@ 2020-10-13 11:32         ` Vladimir Sementsov-Ogievskiy
  2020-10-13 11:49           ` Chenqun (kuhn)
  0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-13 11:32 UTC (permalink / raw)
  To: Chenqun (kuhn), Laurent Vivier, Li Qiang
  Cc: Fam Zheng, ganqixin, Zhanghailiang, qemu-block@nongnu.org,
	Juan Quintela, qemu-trivial@nongnu.org, Qemu Developers,
	Dr. David Alan Gilbert, Stefan Hajnoczi, Euler Robot, Max Reitz,
	John Snow

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-13 11:32         ` Vladimir Sementsov-Ogievskiy
@ 2020-10-13 11:49           ` Chenqun (kuhn)
  0 siblings, 0 replies; 9+ messages in thread
From: Chenqun (kuhn) @ 2020-10-13 11:49 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy, Laurent Vivier, Li Qiang
  Cc: Fam Zheng, Zhanghailiang, qemu-block@nongnu.org, Juan Quintela,
	qemu-trivial@nongnu.org, Stefan Hajnoczi, Qemu Developers,
	Dr. David Alan Gilbert, ganqixin, Euler Robot, Max Reitz,
	John Snow

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
  2020-10-13  1:34   ` Li Qiang
  2020-10-13  7:11     ` Laurent Vivier
@ 2020-10-13  7:27     ` Chenqun (kuhn)
  1 sibling, 0 replies; 9+ messages in thread
From: Chenqun (kuhn) @ 2020-10-13  7:27 UTC (permalink / raw)
  To: Li Qiang, Laurent Vivier
  Cc: Fam Zheng, ganqixin, vsementsov@virtuozzo.com, Zhanghailiang,
	qemu-block@nongnu.org, Juan Quintela, qemu-trivial@nongnu.org,
	Qemu Developers, Dr. David Alan Gilbert, Stefan Hajnoczi,
	Euler Robot, Max Reitz, John Snow

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-10-13 11:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-10 11:07 [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning Chen Qun
2020-10-11  1:55 ` Li Qiang
2020-10-12 15:31 ` Laurent Vivier
2020-10-13  1:34   ` Li Qiang
2020-10-13  7:11     ` Laurent Vivier
2020-10-13  7:49       ` Chenqun (kuhn)
2020-10-13 11:32         ` Vladimir Sementsov-Ogievskiy
2020-10-13 11:49           ` Chenqun (kuhn)
2020-10-13  7:27     ` Chenqun (kuhn)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).