diff mbox series

[5/7] block/vdi: Clean up local variable shadowing

Message ID 20230831132546.3525721-6-armbru@redhat.com (mailing list archive)
State Superseded
Headers show
Series [1/7] migration/rdma: Fix save_page method to fail on polling error | expand

Commit Message

Markus Armbruster Aug. 31, 2023, 1:25 p.m. UTC
Local variables shadowing other local variables or parameters make the
code needlessly hard to understand.  Tracked down with -Wshadow=local.
Clean up: delete inner declarations when they are actually redundant,
else rename variables.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/vdi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Stefan Hajnoczi Aug. 31, 2023, 7:26 p.m. UTC | #1
On Thu, Aug 31, 2023 at 03:25:44PM +0200, Markus Armbruster wrote:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/vdi.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf Sept. 15, 2023, 7:41 a.m. UTC | #2
Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
> Local variables shadowing other local variables or parameters make the
> code needlessly hard to understand.  Tracked down with -Wshadow=local.
> Clean up: delete inner declarations when they are actually redundant,
> else rename variables.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> @@ -700,7 +699,7 @@ nonallocating_write:
>          /* One or more new blocks were allocated. */
>          VdiHeader *header;
>          uint8_t *base;
> -        uint64_t offset;
> +        uint64_t offs;
>          uint32_t n_sectors;
>  
>          g_free(block);
> @@ -723,11 +722,11 @@ nonallocating_write:
>          bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
>          bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
>          n_sectors = bmap_last - bmap_first + 1;
> -        offset = s->bmap_sector + bmap_first;
> +        offs = s->bmap_sector + bmap_first;
>          base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
>          logout("will write %u block map sectors starting from entry %u\n",
>                 n_sectors, bmap_first);
> -        ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
> +        ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
>                               n_sectors * SECTOR_SIZE, base, 0);
>      }

Having two variables 'offset' and 'offs' doesn't really help with
clarity either. Can we be more specific and use something like
'bmap_offset' here?

Kevin
Markus Armbruster Sept. 18, 2023, 2:47 p.m. UTC | #3
Kevin Wolf <kwolf@redhat.com> writes:

> Am 31.08.2023 um 15:25 hat Markus Armbruster geschrieben:
>> Local variables shadowing other local variables or parameters make the
>> code needlessly hard to understand.  Tracked down with -Wshadow=local.
>> Clean up: delete inner declarations when they are actually redundant,
>> else rename variables.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> @@ -700,7 +699,7 @@ nonallocating_write:
>>          /* One or more new blocks were allocated. */
>>          VdiHeader *header;
>>          uint8_t *base;
>> -        uint64_t offset;
>> +        uint64_t offs;
>>          uint32_t n_sectors;
>>  
>>          g_free(block);
>> @@ -723,11 +722,11 @@ nonallocating_write:
>>          bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
>>          bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
>>          n_sectors = bmap_last - bmap_first + 1;
>> -        offset = s->bmap_sector + bmap_first;
>> +        offs = s->bmap_sector + bmap_first;
>>          base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
>>          logout("will write %u block map sectors starting from entry %u\n",
>>                 n_sectors, bmap_first);
>> -        ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
>> +        ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
>>                               n_sectors * SECTOR_SIZE, base, 0);
>>      }
>
> Having two variables 'offset' and 'offs' doesn't really help with
> clarity either. Can we be more specific and use something like
> 'bmap_offset' here?

Sure!
diff mbox series

Patch

diff --git a/block/vdi.c b/block/vdi.c
index 6c35309e04..c084105b78 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,6 @@  vdi_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
         bmap_entry = le32_to_cpu(s->bmap[block_index]);
         if (!VDI_IS_ALLOCATED(bmap_entry)) {
             /* Allocate new block and write to it. */
-            uint64_t data_offset;
             qemu_co_rwlock_upgrade(&s->bmap_lock);
             bmap_entry = le32_to_cpu(s->bmap[block_index]);
             if (VDI_IS_ALLOCATED(bmap_entry)) {
@@ -700,7 +699,7 @@  nonallocating_write:
         /* One or more new blocks were allocated. */
         VdiHeader *header;
         uint8_t *base;
-        uint64_t offset;
+        uint64_t offs;
         uint32_t n_sectors;
 
         g_free(block);
@@ -723,11 +722,11 @@  nonallocating_write:
         bmap_first /= (SECTOR_SIZE / sizeof(uint32_t));
         bmap_last /= (SECTOR_SIZE / sizeof(uint32_t));
         n_sectors = bmap_last - bmap_first + 1;
-        offset = s->bmap_sector + bmap_first;
+        offs = s->bmap_sector + bmap_first;
         base = ((uint8_t *)&s->bmap[0]) + bmap_first * SECTOR_SIZE;
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
-        ret = bdrv_co_pwrite(bs->file, offset * SECTOR_SIZE,
+        ret = bdrv_co_pwrite(bs->file, offs * SECTOR_SIZE,
                              n_sectors * SECTOR_SIZE, base, 0);
     }