* [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() @ 2025-10-07 22:27 Sean Christopherson 2025-10-09 23:46 ` Ackerley Tng 2025-10-15 18:02 ` Sean Christopherson 0 siblings, 2 replies; 6+ messages in thread From: Sean Christopherson @ 2025-10-07 22:27 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, linux-kernel, Sean Christopherson Drop the local "int err" that's buried in the middle guest_memfd's user fault handler to avoid the potential for variable shadowing, e.g. if an "err" variable were also declared at function scope. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- virt/kvm/guest_memfd.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index 94bafd6c558c..abbec01d7a3a 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -330,12 +330,10 @@ static vm_fault_t kvm_gmem_fault_user_mapping(struct vm_fault *vmf) folio = kvm_gmem_get_folio(inode, vmf->pgoff); if (IS_ERR(folio)) { - int err = PTR_ERR(folio); - - if (err == -EAGAIN) + if (PTR_ERR(folio) == -EAGAIN) return VM_FAULT_RETRY; - return vmf_error(err); + return vmf_error(PTR_ERR(folio)); } if (WARN_ON_ONCE(folio_test_large(folio))) { base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac -- 2.51.0.710.ga91ca5db03-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() 2025-10-07 22:27 [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() Sean Christopherson @ 2025-10-09 23:46 ` Ackerley Tng 2025-10-10 22:11 ` Sean Christopherson 2025-10-15 18:02 ` Sean Christopherson 1 sibling, 1 reply; 6+ messages in thread From: Ackerley Tng @ 2025-10-09 23:46 UTC (permalink / raw) To: Sean Christopherson; +Cc: pbonzini, kvm, linux-kernel, seanjc Sean Christopherson <seanjc@google.com> writes: > Drop the local "int err" that's buried in the middle guest_memfd's user > fault handler to avoid the potential for variable shadowing, e.g. if an > "err" variable were also declared at function scope. > Is the takeaway here that the variable name "err", if used, should be defined at function scope? IOW, would this code have been okay if any other variable name were used, like if err_folio were used instead of err? > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Ackerley Tng <ackerleytng@google.com> > --- > virt/kvm/guest_memfd.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > index 94bafd6c558c..abbec01d7a3a 100644 > --- a/virt/kvm/guest_memfd.c > +++ b/virt/kvm/guest_memfd.c > @@ -330,12 +330,10 @@ static vm_fault_t kvm_gmem_fault_user_mapping(struct vm_fault *vmf) > > folio = kvm_gmem_get_folio(inode, vmf->pgoff); > if (IS_ERR(folio)) { > - int err = PTR_ERR(folio); > - > - if (err == -EAGAIN) > + if (PTR_ERR(folio) == -EAGAIN) > return VM_FAULT_RETRY; > > - return vmf_error(err); > + return vmf_error(PTR_ERR(folio)); > } > > if (WARN_ON_ONCE(folio_test_large(folio))) { > > base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() 2025-10-09 23:46 ` Ackerley Tng @ 2025-10-10 22:11 ` Sean Christopherson 2025-10-10 22:33 ` Ackerley Tng 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2025-10-10 22:11 UTC (permalink / raw) To: Ackerley Tng; +Cc: pbonzini, kvm, linux-kernel On Thu, Oct 09, 2025, Ackerley Tng wrote: > Sean Christopherson <seanjc@google.com> writes: > > > Drop the local "int err" that's buried in the middle guest_memfd's user > > fault handler to avoid the potential for variable shadowing, e.g. if an > > "err" variable were also declared at function scope. > > > > Is the takeaway here that the variable name "err", if used, should be > defined at function scope? Generally speaking, for generic variables like "err", "r", and "ret", yes, because the danger of shadowing is high, while the odds of _wanting_ to contain a return code are low. But as a broad rule, there's simply no "right" answer other than "it depends". As Paolo put it, damned if you do, damned if you don't. See this thread for more: https://lore.kernel.org/all/YZL1ZiKQVRQd8rZi@google.com > IOW, would this code have been okay if any other variable name were > used, like if err_folio were used instead of err? Probably not? Because that has it's own problems. E.g. then you can end up introducing bugs like this: int err; err = blah(); if (err) goto fault_err; folio = kvm_gmem_get_folio(inode, vmf->pgoff); if (IS_ERR(folio)) { int folio_err = PTR_ERR(folio); if (folio_err == -EAGAIN) return VM_FAULT_RETRY; goto fault_err; } ... fault_err: return vmf_error(err); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() 2025-10-10 22:11 ` Sean Christopherson @ 2025-10-10 22:33 ` Ackerley Tng 0 siblings, 0 replies; 6+ messages in thread From: Ackerley Tng @ 2025-10-10 22:33 UTC (permalink / raw) To: Sean Christopherson; +Cc: pbonzini, kvm, linux-kernel Sean Christopherson <seanjc@google.com> writes: > On Thu, Oct 09, 2025, Ackerley Tng wrote: >> Sean Christopherson <seanjc@google.com> writes: >> >> > Drop the local "int err" that's buried in the middle guest_memfd's user >> > fault handler to avoid the potential for variable shadowing, e.g. if an >> > "err" variable were also declared at function scope. >> > >> >> Is the takeaway here that the variable name "err", if used, should be >> defined at function scope? > > Generally speaking, for generic variables like "err", "r", and "ret", yes, because > the danger of shadowing is high, while the odds of _wanting_ to contain a return > code are low. > > But as a broad rule, there's simply no "right" answer other than "it depends". > As Paolo put it, damned if you do, damned if you don't. See this thread for more: > > https://lore.kernel.org/all/YZL1ZiKQVRQd8rZi@google.com > >> IOW, would this code have been okay if any other variable name were >> used, like if err_folio were used instead of err? > > Probably not? Because that has it's own problems. E.g. then you can end up > introducing bugs like this: > > int err; > > err = blah(); > if (err) > goto fault_err; > > folio = kvm_gmem_get_folio(inode, vmf->pgoff); > if (IS_ERR(folio)) { > int folio_err = PTR_ERR(folio); > > if (folio_err == -EAGAIN) > return VM_FAULT_RETRY; > > goto fault_err; > } > > ... > > fault_err: > return vmf_error(err); This is true too. Thanks, I see why it depends. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() 2025-10-07 22:27 [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() Sean Christopherson 2025-10-09 23:46 ` Ackerley Tng @ 2025-10-15 18:02 ` Sean Christopherson 2025-10-20 15:51 ` Sean Christopherson 1 sibling, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2025-10-15 18:02 UTC (permalink / raw) To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel On Tue, 07 Oct 2025 15:27:33 -0700, Sean Christopherson wrote: > Drop the local "int err" that's buried in the middle guest_memfd's user > fault handler to avoid the potential for variable shadowing, e.g. if an > "err" variable were also declared at function scope. > > No functional change intended. > > > [...] Applied to kvm-x86 gmem, thanks! [1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() https://github.com/kvm-x86/linux/commit/c1168f24b444 -- https://github.com/kvm-x86/linux/tree/next ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() 2025-10-15 18:02 ` Sean Christopherson @ 2025-10-20 15:51 ` Sean Christopherson 0 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2025-10-20 15:51 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, linux-kernel On Wed, Oct 15, 2025, Sean Christopherson wrote: > On Tue, 07 Oct 2025 15:27:33 -0700, Sean Christopherson wrote: > > Drop the local "int err" that's buried in the middle guest_memfd's user > > fault handler to avoid the potential for variable shadowing, e.g. if an > > "err" variable were also declared at function scope. > > > > No functional change intended. > > > > > > [...] > > Applied to kvm-x86 gmem, thanks! > > [1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() > https://github.com/kvm-x86/linux/commit/c1168f24b444 FYI, I rebased this onto 6.18-rc2 to avoid a silly merge. New hash: [1/1] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() https://github.com/kvm-x86/linux/commit/5f3e10797ab8 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-20 15:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-07 22:27 [PATCH] KVM: guest_memfd: Drop a superfluous local var in kvm_gmem_fault_user_mapping() Sean Christopherson 2025-10-09 23:46 ` Ackerley Tng 2025-10-10 22:11 ` Sean Christopherson 2025-10-10 22:33 ` Ackerley Tng 2025-10-15 18:02 ` Sean Christopherson 2025-10-20 15:51 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox