Re: [PATCH 6/7] block: 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
>> ---
>> 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, _attach_child_common_drv, s);
>> +tran_add(transaction, _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!
Re: [PATCH 6/7] block: 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
> ---
> 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, _attach_child_common_drv, s);
> +tran_add(transaction, _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
Re: [PATCH 6/7] block: Clean up local variable shadowing
On Thu, Aug 31, 2023 at 3:25 PM 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. For rbd > block/rbd.c | 2 +- Acked-by: Ilya Dryomov Thanks, Ilya
Re: [PATCH 6/7] block: Clean up local variable shadowing
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
Thanks,
--
Anthony PERARD
Re: [PATCH 6/7] block: Clean up local variable shadowing
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 > --- > 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 signature.asc Description: PGP signature
[PATCH 6/7] block: 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.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(-)
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, _attach_child_common_drv, s);
+tran_add(transaction, _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, _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,
