diff mbox series

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

Message ID 20201013123340.1850548-1-kuhn.chenqun@huawei.com
State New
Headers show
Series [v2] migration/block-dirty-bitmap: fix uninitialized variable warning | expand

Commit Message

Chenqun (kuhn) Oct. 13, 2020, 12:33 p.m. UTC
A default value is provided for the variable 'bitmap_name' to avoid compiler warning.

The compiler show 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>
---
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Li Qiang <liq3ea@gmail.com>
---
 migration/block-dirty-bitmap.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

Comments

Max Reitz Oct. 13, 2020, 2:47 p.m. UTC | #1
On 13.10.20 14:33, Chen Qun wrote:
> A default value is provided for the variable 'bitmap_name' to avoid compiler warning.
> 
> The compiler show 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>
> ---
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: Li Qiang <liq3ea@gmail.com>
> ---
>  migration/block-dirty-bitmap.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)

No objections, semantically, but...

> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 5bef793ac0..bcb79c04ce 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -1064,15 +1064,13 @@ 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;
> -
>          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) {
> +        const char *bitmap_name = s->bitmap_alias;

qemu’s coding style mandates declarations to be placed at the beginning
of their block, so the declaration has to stay where it is.  (Putting
the assignment here looks reasonable.)

> +        if (!s->cancelled && bitmap_alias_map) {
>                  bitmap_name = g_hash_table_lookup(bitmap_alias_map,
>                                                    s->bitmap_alias);

This block of course needs to be re-indented.

Max

>                  if (!bitmap_name) {
> @@ -1081,9 +1079,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) {
>
Chenqun (kuhn) Oct. 14, 2020, 1:03 a.m. UTC | #2
> -----Original Message-----
> From: Max Reitz [mailto:mreitz@redhat.com]
> Sent: Tuesday, October 13, 2020 10:47 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: vsementsov@virtuozzo.com; stefanha@redhat.com; fam@euphon.net;
> eblake@redhat.com; jsnow@redhat.com; quintela@redhat.com;
> dgilbert@redhat.com; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> ganqixin <ganqixin@huawei.com>; qemu-block@nongnu.org; Euler Robot
> <euler.robot@huawei.com>; Laurent Vivier <laurent@vivier.eu>; Li Qiang
> <liq3ea@gmail.com>
> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized variable
> warning
> 
> On 13.10.20 14:33, Chen Qun wrote:
> > A default value is provided for the variable 'bitmap_name' to avoid compiler
> warning.
> >
> > The compiler show 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>
> > ---
> > Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > Cc: Laurent Vivier <laurent@vivier.eu>
> > Cc: Li Qiang <liq3ea@gmail.com>
> > ---
> >  migration/block-dirty-bitmap.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> No objections, semantically, but...
> 
> > diff --git a/migration/block-dirty-bitmap.c
> > b/migration/block-dirty-bitmap.c index 5bef793ac0..bcb79c04ce 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1064,15 +1064,13 @@ 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;
> > -
> >          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) {
> > +        const char *bitmap_name = s->bitmap_alias;
> 
> qemu’s coding style mandates declarations to be placed at the beginning of
> their block, so the declaration has to stay where it is.  (Putting the assignment
> here looks reasonable.)
> 
Hi Max,
  Declaration variables here to ensure that the above exceptions(Unable to read bitmap alias string) are avoided.
If the declaration has to stay where it is, is there a possibility that the assignment fails?

> > +        if (!s->cancelled && bitmap_alias_map) {
> >                  bitmap_name =
> g_hash_table_lookup(bitmap_alias_map,
> >
> s->bitmap_alias);
> 
> This block of course needs to be re-indented.
> 
I forgot this. I will update it later.

Thanks,
ChenQun

> 
> >                  if (!bitmap_name) {
> > @@ -1081,9 +1079,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) {
> >
>
Max Reitz Oct. 14, 2020, 9:35 a.m. UTC | #3
On 14.10.20 03:03, Chenqun (kuhn) wrote:
> 
> 
>> -----Original Message-----
>> From: Max Reitz [mailto:mreitz@redhat.com]
>> Sent: Tuesday, October 13, 2020 10:47 PM
>> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
>> qemu-trivial@nongnu.org
>> Cc: vsementsov@virtuozzo.com; stefanha@redhat.com; fam@euphon.net;
>> eblake@redhat.com; jsnow@redhat.com; quintela@redhat.com;
>> dgilbert@redhat.com; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
>> ganqixin <ganqixin@huawei.com>; qemu-block@nongnu.org; Euler Robot
>> <euler.robot@huawei.com>; Laurent Vivier <laurent@vivier.eu>; Li Qiang
>> <liq3ea@gmail.com>
>> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized variable
>> warning
>>
>> On 13.10.20 14:33, Chen Qun wrote:
>>> A default value is provided for the variable 'bitmap_name' to avoid compiler
>> warning.
>>>
>>> The compiler show 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>
>>> ---
>>> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>> Cc: Li Qiang <liq3ea@gmail.com>
>>> ---
>>>  migration/block-dirty-bitmap.c | 9 ++-------
>>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> No objections, semantically, but...
>>
>>> diff --git a/migration/block-dirty-bitmap.c
>>> b/migration/block-dirty-bitmap.c index 5bef793ac0..bcb79c04ce 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -1064,15 +1064,13 @@ 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;
>>> -
>>>          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) {
>>> +        const char *bitmap_name = s->bitmap_alias;
>>
>> qemu’s coding style mandates declarations to be placed at the beginning of
>> their block, so the declaration has to stay where it is.  (Putting the assignment
>> here looks reasonable.)
>>
> Hi Max,
>   Declaration variables here to ensure that the above exceptions(Unable to read bitmap alias string) are avoided.
> If the declaration has to stay where it is, is there a possibility that the assignment fails?

I don’t understand what you mean.  A declaration without initialization
isn’t and doesn’t contain an expression, it isn’t even a statement, so
it has no side effects.[1]

Placing the declaration (without an initialization) at the top of the
block makes no semantic difference.

(As I said, I’d keep the assignment “bitmap_name = s->bitmap_alias”
where you put it.  I think it would technically actually be correct to
put it into the declaration at the start of the block as an initializer,
but that would look weird.)

Max

[1] I suppose exceptions apply for types with constructors, but those
don’t exist in plain C.
Chenqun (kuhn) Oct. 14, 2020, 11:21 a.m. UTC | #4
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+kuhn.chenqun=huawei.com@nongnu.org] On
> Behalf Of Max Reitz
> Sent: Wednesday, October 14, 2020 5:36 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: fam@euphon.net; ganqixin <ganqixin@huawei.com>;
> vsementsov@virtuozzo.com; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; qemu-block@nongnu.org;
> quintela@redhat.com; Li Qiang <liq3ea@gmail.com>; dgilbert@redhat.com;
> Laurent Vivier <laurent@vivier.eu>; stefanha@redhat.com; Euler Robot
> <euler.robot@huawei.com>; jsnow@redhat.com
> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized variable
> warning
> 
> On 14.10.20 03:03, Chenqun (kuhn) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Max Reitz [mailto:mreitz@redhat.com]
> >> Sent: Tuesday, October 13, 2020 10:47 PM
> >> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>;
> qemu-devel@nongnu.org;
> >> qemu-trivial@nongnu.org
> >> Cc: vsementsov@virtuozzo.com; stefanha@redhat.com; fam@euphon.net;
> >> eblake@redhat.com; jsnow@redhat.com; quintela@redhat.com;
> >> dgilbert@redhat.com; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> >> ganqixin <ganqixin@huawei.com>; qemu-block@nongnu.org; Euler Robot
> >> <euler.robot@huawei.com>; Laurent Vivier <laurent@vivier.eu>; Li
> >> Qiang <liq3ea@gmail.com>
> >> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix
> >> uninitialized variable warning
> >>
> >> On 13.10.20 14:33, Chen Qun wrote:
> >>> A default value is provided for the variable 'bitmap_name' to avoid
> >>> compiler
> >> warning.
> >>>
> >>> The compiler show 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>
> >>> ---
> >>> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>> Cc: Laurent Vivier <laurent@vivier.eu>
> >>> Cc: Li Qiang <liq3ea@gmail.com>
> >>> ---
> >>>  migration/block-dirty-bitmap.c | 9 ++-------
> >>>  1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> No objections, semantically, but...
> >>
> >>> diff --git a/migration/block-dirty-bitmap.c
> >>> b/migration/block-dirty-bitmap.c index 5bef793ac0..bcb79c04ce 100644
> >>> --- a/migration/block-dirty-bitmap.c
> >>> +++ b/migration/block-dirty-bitmap.c
> >>> @@ -1064,15 +1064,13 @@ 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;
> >>> -
> >>>          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) {
> >>> +        const char *bitmap_name = s->bitmap_alias;
> >>
> >> qemu’s coding style mandates declarations to be placed at the
> >> beginning of their block, so the declaration has to stay where it is.
> >> (Putting the assignment here looks reasonable.)
> >>
> > Hi Max,
> >   Declaration variables here to ensure that the above exceptions(Unable to
> read bitmap alias string) are avoided.
> > If the declaration has to stay where it is, is there a possibility that the
> assignment fails?
> 
> I don’t understand what you mean.  

I think my description is not accurate. Forgive me for being a non-native English speaker.
The variable 'bitmap_name' assignment maybe failed at the beginning of the block, because reading the 's->bitmap_alias' maybe failed.

>A declaration without initialization isn’t
> and doesn’t contain an expression, it isn’t even a statement, so it has no side
> effects.[1]
> 
> Placing the declaration (without an initialization) at the top of the block makes
> no semantic difference.
>
I see what you mean. Separate variable declarations from variable assignments.
You're right!  I will update it later.

Thanks,
Chen Qun
> (As I said, I’d keep the assignment “bitmap_name = s->bitmap_alias”
> where you put it.  I think it would technically actually be correct to put it into
> the declaration at the start of the block as an initializer, but that would look
> weird.)
> 
> Max
> 
> [1] I suppose exceptions apply for types with constructors, but those don’t
> exist in plain C.
diff mbox series

Patch

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 5bef793ac0..bcb79c04ce 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -1064,15 +1064,13 @@  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;
-
         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) {
+        const char *bitmap_name = s->bitmap_alias;
+        if (!s->cancelled && bitmap_alias_map) {
                 bitmap_name = g_hash_table_lookup(bitmap_alias_map,
                                                   s->bitmap_alias);
                 if (!bitmap_name) {
@@ -1081,9 +1079,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) {