Re: [PATCH 5/7] block/vdi: Clean up local variable shadowing
Kevin Wolf 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
>
>> @@ -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 *)>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!
Re: [PATCH 5/7] block/vdi: Clean up local variable shadowing
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
> @@ -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 *)>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
Re: [PATCH 5/7] block/vdi: Clean up local variable shadowing
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 > --- > block/vdi.c | 7 +++ > 1 file changed, 3 insertions(+), 4 deletions(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
[PATCH 5/7] block/vdi: Clean up local variable shadowing
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
---
block/vdi.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
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(>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 *)>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);
}
--
2.41.0
