diff mbox series

[6/7] block: Clean up local variable shadowing

Message ID 20230831132546.3525721-7-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.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(-)

Comments

Stefan Hajnoczi Aug. 31, 2023, 7:24 p.m. UTC | #1
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>
Anthony PERARD Sept. 11, 2023, 10:44 a.m. UTC | #2
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,
Ilya Dryomov Sept. 11, 2023, 11:17 a.m. UTC | #3
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
Kevin Wolf Sept. 15, 2023, 8:10 a.m. UTC | #4
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
Markus Armbruster Sept. 18, 2023, 2:49 p.m. UTC | #5
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 mbox series

Patch

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: