| Message ID | 20230831132546.3525721-6-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] migration/rdma: Fix save_page method to fail on polling error | expand |
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>
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
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 --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); }
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(-)