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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3560 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]