| Message ID | 20230831132546.3525721-7-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:45PM +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.c | 7 ++++--- > block/rbd.c | 2 +- > block/stream.c | 1 - > block/vvfat.c | 34 +++++++++++++++++----------------- > hw/block/xen-block.c | 6 +++--- > 5 files changed, 25 insertions(+), 25 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Thu, Aug 31, 2023 at 03:25:45PM +0200, Markus Armbruster wrote: > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c > index 3906b9058b..a07cd7eb5d 100644 > --- a/hw/block/xen-block.c > +++ b/hw/block/xen-block.c > @@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name, > case XEN_BLOCK_VDEV_TYPE_XVD: > case XEN_BLOCK_VDEV_TYPE_HD: > case XEN_BLOCK_VDEV_TYPE_SD: { > - char *name = disk_to_vbd_name(vdev->disk); > + char *vbd_name = disk_to_vbd_name(vdev->disk); > > str = g_strdup_printf("%s%s%lu", > (vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ? > @@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name, > (vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ? > "hd" : > "sd", > - name, vdev->partition); > - g_free(name); > + vbd_name, vdev->partition); > + g_free(vbd_name); > break; > } > default: Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
On Thu, Aug 31, 2023 at 3:25 PM Markus Armbruster <armbru@redhat.com> 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. For rbd > block/rbd.c | 2 +- Acked-by: Ilya Dryomov <idryomov@gmail.com> Thanks, Ilya
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> > --- > block.c | 7 ++++--- > block/rbd.c | 2 +- > block/stream.c | 1 - > block/vvfat.c | 34 +++++++++++++++++----------------- > hw/block/xen-block.c | 6 +++--- > 5 files changed, 25 insertions(+), 25 deletions(-) I wonder why you made vdi a separate patch, but not vvfat, even though that has more changes. (Of course, my selfish motivation for asking this is that I could have given a R-b for it and wouldn't have to look at it again in a v2 :-)) > diff --git a/block.c b/block.c > index a307c151a8..7f0003d8ac 100644 > --- a/block.c > +++ b/block.c > @@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, > BdrvChildRole child_role, > uint64_t perm, uint64_t shared_perm, > void *opaque, > - Transaction *tran, Error **errp) > + Transaction *transaction, > + Error **errp) > { > BdrvChild *new_child; > AioContext *parent_ctx, *new_child_ctx; > @@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, > .old_parent_ctx = parent_ctx, > .old_child_ctx = child_ctx, > }; > - tran_add(tran, &bdrv_attach_child_common_drv, s); > + tran_add(transaction, &bdrv_attach_child_common_drv, s); > > if (new_child_ctx != child_ctx) { > aio_context_release(new_child_ctx); I think I would resolve this one the other way around. 'tran' is the typical name for the parameter and it is the transaction that this function should add things to. The other one that shadows it is a local transaction that is completed within the function. I think it's better if that one has a different name. As usual, being more specific than just 'tran' vs. 'transaction' would be nice. Maybe 'aio_ctx_tran' for the nested one? The rest looks okay. 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> >> --- >> block.c | 7 ++++--- >> block/rbd.c | 2 +- >> block/stream.c | 1 - >> block/vvfat.c | 34 +++++++++++++++++----------------- >> hw/block/xen-block.c | 6 +++--- >> 5 files changed, 25 insertions(+), 25 deletions(-) > > I wonder why you made vdi a separate patch, but not vvfat, even though > that has more changes. (Of course, my selfish motivation for asking this > is that I could have given a R-b for it and wouldn't have to look at it > again in a v2 :-)) I split by maintainer. The files changed by this patch are only covered by "Block layer core". >> diff --git a/block.c b/block.c >> index a307c151a8..7f0003d8ac 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, >> BdrvChildRole child_role, >> uint64_t perm, uint64_t shared_perm, >> void *opaque, >> - Transaction *tran, Error **errp) >> + Transaction *transaction, >> + Error **errp) >> { >> BdrvChild *new_child; >> AioContext *parent_ctx, *new_child_ctx; >> @@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, >> .old_parent_ctx = parent_ctx, >> .old_child_ctx = child_ctx, >> }; >> - tran_add(tran, &bdrv_attach_child_common_drv, s); >> + tran_add(transaction, &bdrv_attach_child_common_drv, s); >> >> if (new_child_ctx != child_ctx) { >> aio_context_release(new_child_ctx); > > I think I would resolve this one the other way around. 'tran' is the > typical name for the parameter and it is the transaction that this > function should add things to. > > The other one that shadows it is a local transaction that is completed > within the function. I think it's better if that one has a different > name. > > As usual, being more specific than just 'tran' vs. 'transaction' would > be nice. Maybe 'aio_ctx_tran' for the nested one? Can do. > The rest looks okay. Thanks!
diff --git a/block.c b/block.c index a307c151a8..7f0003d8ac 100644 --- a/block.c +++ b/block.c @@ -3001,7 +3001,8 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, BdrvChildRole child_role, uint64_t perm, uint64_t shared_perm, void *opaque, - Transaction *tran, Error **errp) + Transaction *transaction, + Error **errp) { BdrvChild *new_child; AioContext *parent_ctx, *new_child_ctx; @@ -3088,7 +3089,7 @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs, .old_parent_ctx = parent_ctx, .old_child_ctx = child_ctx, }; - tran_add(tran, &bdrv_attach_child_common_drv, s); + tran_add(transaction, &bdrv_attach_child_common_drv, s); if (new_child_ctx != child_ctx) { aio_context_release(new_child_ctx); @@ -6073,12 +6074,12 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name), QLIST_FOREACH(drv, &bdrv_drivers, list) { if (drv->format_name) { bool found = false; - int i = count; if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) { continue; } + i = count; while (formats && i && !found) { found = !strcmp(formats[--i], drv->format_name); } diff --git a/block/rbd.c b/block/rbd.c index 978671411e..472ca05cba 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -1290,7 +1290,7 @@ static int coroutine_fn qemu_rbd_start_co(BlockDriverState *bs, * operations that exceed the current size. */ if (offset + bytes > s->image_size) { - int r = qemu_rbd_resize(bs, offset + bytes); + r = qemu_rbd_resize(bs, offset + bytes); if (r < 0) { return r; } diff --git a/block/stream.c b/block/stream.c index e522bbdec5..007253880b 100644 --- a/block/stream.c +++ b/block/stream.c @@ -282,7 +282,6 @@ void stream_start(const char *job_id, BlockDriverState *bs, /* Make sure that the image is opened in read-write mode */ bs_read_only = bdrv_is_read_only(bs); if (bs_read_only) { - int ret; /* Hold the chain during reopen */ if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) { return; diff --git a/block/vvfat.c b/block/vvfat.c index 0ddc91fc09..d7425ee602 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -777,7 +777,6 @@ static int read_directory(BDRVVVFATState* s, int mapping_index) while((entry=readdir(dir))) { unsigned int length=strlen(dirname)+2+strlen(entry->d_name); char* buffer; - direntry_t* direntry; struct stat st; int is_dot=!strcmp(entry->d_name,"."); int is_dotdot=!strcmp(entry->d_name,".."); @@ -857,7 +856,7 @@ static int read_directory(BDRVVVFATState* s, int mapping_index) /* fill with zeroes up to the end of the cluster */ while(s->directory.next%(0x10*s->sectors_per_cluster)) { - direntry_t* direntry=array_get_next(&(s->directory)); + direntry = array_get_next(&(s->directory)); memset(direntry,0,sizeof(direntry_t)); } @@ -1962,24 +1961,24 @@ get_cluster_count_for_direntry(BDRVVVFATState* s, direntry_t* direntry, const ch * This is horribly inefficient, but that is okay, since * it is rarely executed, if at all. */ - int64_t offset = cluster2sector(s, cluster_num); + int64_t offs = cluster2sector(s, cluster_num); vvfat_close_current_file(s); for (i = 0; i < s->sectors_per_cluster; i++) { int res; res = bdrv_is_allocated(s->qcow->bs, - (offset + i) * BDRV_SECTOR_SIZE, + (offs + i) * BDRV_SECTOR_SIZE, BDRV_SECTOR_SIZE, NULL); if (res < 0) { return -1; } if (!res) { - res = vvfat_read(s->bs, offset, s->cluster_buffer, 1); + res = vvfat_read(s->bs, offs, s->cluster_buffer, 1); if (res) { return -1; } - res = bdrv_co_pwrite(s->qcow, offset * BDRV_SECTOR_SIZE, + res = bdrv_co_pwrite(s->qcow, offs * BDRV_SECTOR_SIZE, BDRV_SECTOR_SIZE, s->cluster_buffer, 0); if (res < 0) { @@ -2467,8 +2466,9 @@ commit_direntries(BDRVVVFATState* s, int dir_index, int parent_mapping_index) for (c = first_cluster; !fat_eof(s, c); c = modified_fat_get(s, c)) { direntry_t *first_direntry; - void* direntry = array_get(&(s->directory), current_dir_index); - int ret = vvfat_read(s->bs, cluster2sector(s, c), direntry, + + direntry = array_get(&(s->directory), current_dir_index); + ret = vvfat_read(s->bs, cluster2sector(s, c), (uint8_t *)direntry, s->sectors_per_cluster); if (ret) return ret; @@ -2690,12 +2690,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s) direntry_t* direntry = array_get(&(s->directory), mapping->info.dir.first_dir_index); uint32_t c = mapping->begin; - int i = 0; + int j = 0; /* recurse */ while (!fat_eof(s, c)) { do { - direntry_t* d = direntry + i; + direntry_t* d = direntry + j; if (is_file(d) || (is_directory(d) && !is_dot(d))) { int l; @@ -2716,8 +2716,8 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s) schedule_rename(s, m->begin, new_path); } - i++; - } while((i % (0x10 * s->sectors_per_cluster)) != 0); + j++; + } while((j % (0x10 * s->sectors_per_cluster)) != 0); c = fat_get(s, c); } } @@ -2804,16 +2804,16 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s) int begin = commit->param.new_file.first_cluster; mapping_t* mapping = find_mapping_for_cluster(s, begin); direntry_t* entry; - int i; + int j; /* find direntry */ - for (i = 0; i < s->directory.next; i++) { - entry = array_get(&(s->directory), i); + for (j = 0; j < s->directory.next; j++) { + entry = array_get(&(s->directory), j); if (is_file(entry) && begin_of_direntry(entry) == begin) break; } - if (i >= s->directory.next) { + if (j >= s->directory.next) { fail = -6; continue; } @@ -2833,7 +2833,7 @@ static int coroutine_fn GRAPH_RDLOCK handle_commits(BDRVVVFATState* s) mapping->mode = MODE_NORMAL; mapping->info.file.offset = 0; - if (commit_one_file(s, i, 0)) + if (commit_one_file(s, j, 0)) fail = -7; break; diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c index 3906b9058b..a07cd7eb5d 100644 --- a/hw/block/xen-block.c +++ b/hw/block/xen-block.c @@ -369,7 +369,7 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name, case XEN_BLOCK_VDEV_TYPE_XVD: case XEN_BLOCK_VDEV_TYPE_HD: case XEN_BLOCK_VDEV_TYPE_SD: { - char *name = disk_to_vbd_name(vdev->disk); + char *vbd_name = disk_to_vbd_name(vdev->disk); str = g_strdup_printf("%s%s%lu", (vdev->type == XEN_BLOCK_VDEV_TYPE_XVD) ? @@ -377,8 +377,8 @@ static void xen_block_get_vdev(Object *obj, Visitor *v, const char *name, (vdev->type == XEN_BLOCK_VDEV_TYPE_HD) ? "hd" : "sd", - name, vdev->partition); - g_free(name); + vbd_name, vdev->partition); + g_free(vbd_name); break; } default:
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.c | 7 ++++--- block/rbd.c | 2 +- block/stream.c | 1 - block/vvfat.c | 34 +++++++++++++++++----------------- hw/block/xen-block.c | 6 +++--- 5 files changed, 25 insertions(+), 25 deletions(-)