| Message ID | 20201013123340.1850548-1-kuhn.chenqun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] migration/block-dirty-bitmap: fix uninitialized variable warning | expand |
> -----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) { > > >
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.
> -----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 --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) {
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(-)