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;
| ^~~~
Signed-off-by: Qian Cai <[email protected]>
---
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;
- if (gfn >= memslots[slot].base_gfn)
- end = slot;
+ if (gfn >= memslots[new_slot].base_gfn)
+ end = new_slot;
else
- start = slot + 1;
+ start = new_slot + 1;
}
slot = try_get_memslot(slots, start, gfn);
--
2.30.2
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 <[email protected]>
> ---
> 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);
On 10/26/21 11:44 AM, Sean Christopherson wrote:
> 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 <[email protected]>
>> ---
>> 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.
Yes, I like "pivot" and the rest of the feedback makes sense. I'll send
a v2 soon.
On 10/26/21 11:44 AM, Sean Christopherson wrote:
> 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.
Actually, I am a bit more prefer to keep the declaration inside the loop
as it makes the declaration and assignment closer to make it easier to
understand the code. It should be relatively trivial to avoid potential
shadows in the future. It would be interesting to see what Paolo would say.
On 26/10/21 18:14, Qian Cai wrote:
>> 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.
> Actually, I am a bit more prefer to keep the declaration inside the loop
> as it makes the declaration and assignment closer to make it easier to
> understand the code. It should be relatively trivial to avoid potential
> shadows in the future. It would be interesting to see what Paolo would say.
You both have good arguments, so whoever writes the patch wins. :)
Paolo
On Tue, Oct 26, 2021, Paolo Bonzini wrote:
> On 26/10/21 18:14, Qian Cai wrote:
> > > 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.
> > Actually, I am a bit more prefer to keep the declaration inside the loop
> > as it makes the declaration and assignment closer to make it easier to
> > understand the code. It should be relatively trivial to avoid potential
> > shadows in the future. It would be interesting to see what Paolo would say.
>
> You both have good arguments, so whoever writes the patch wins. :)
LOL, KVM's version of Thunderdome.