Re: [PATCH] kvm: Avoid shadowing a local in search_memslots()

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Tue, Oct 26, 2021, Qian Cai wrote:
> It is less error-prone to use a different variable name from the existing
> one in a wider scope. This is also flagged by GCC (W=2):
> 
> ./include/linux/kvm_host.h: In function 'search_memslots':
> ./include/linux/kvm_host.h:1246:7: warning: declaration of 'slot' shadows a previous local [-Wshadow]
>  1246 |   int slot = start + (end - start) / 2;
>       |       ^~~~
> ./include/linux/kvm_host.h:1240:26: note: shadowed declaration is here
>  1240 |  struct kvm_memory_slot *slot;
>       |                          ^~~~
> 

Even though this doesn't need to go to stable, probably worth adding a Fixes: to
acknowledge that this was a recently introduced mess.

  Fixes: 0f22af940dc8 ("KVM: Move last_used_slot logic out of search_memslots")


> Signed-off-by: Qian Cai <quic_qiancai@xxxxxxxxxxx>
> ---
>  include/linux/kvm_host.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 60a35d9fe259..1c1a36f658fe 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1243,12 +1243,12 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
>  		return NULL;
>  
>  	while (start < end) {
> -		int slot = start + (end - start) / 2;
> +		int new_slot = start + (end - start) / 2;

new_slot isn't a great name, the integer "slot" isn't directly connected to the
final memslot and may not be representative of the final memslot's index depending
on how the binary search resolves.

Maybe "pivot"?  Or just "tmp"?  I also vote to hoist the declaration out of the
loop precisely to avoid potential shadows, and to also associate the variable
with the "start" and "end" variables, e.g.

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 60a35d9fe259..663bdfa0983f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1235,7 +1235,7 @@ try_get_memslot(struct kvm_memslots *slots, int slot_index, gfn_t gfn)
 static inline struct kvm_memory_slot *
 search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
 {
-       int start = 0, end = slots->used_slots;
+       int start = 0, end = slots->used_slots, pivot;
        struct kvm_memory_slot *memslots = slots->memslots;
        struct kvm_memory_slot *slot;

@@ -1243,12 +1243,11 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn, int *index)
                return NULL;

        while (start < end) {
-               int slot = start + (end - start) / 2;
-
-               if (gfn >= memslots[slot].base_gfn)
-                       end = slot;
+               pivot = start + (end - start) / 2;
+               if (gfn >= memslots[pivot].base_gfn)
+                       end = pivot;
                else
-                       start = slot + 1;
+                       start = pivot + 1;
        }

        slot = try_get_memslot(slots, start, gfn);