* [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl()
@ 2020-02-28 5:25 Xiaoyao Li
2020-02-28 15:44 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Xiaoyao Li @ 2020-02-28 5:25 UTC (permalink / raw)
To: Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
Jim Mattson, Joerg Roedel
Cc: Xiaoyao Li, kvm, linux-kernel
Remove unnecessary brackets from the case statements in
kvm_arch_dev_ioctl().
The brackets are visually confusing and error-prone, e.g., brackets of
case KVM_X86_GET_MCE_CAP_SUPPORTED accidently includes case
KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/x86.c | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ddd1d296bd20..9efd693189df 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3412,14 +3412,16 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
long kvm_arch_dev_ioctl(struct file *filp,
unsigned int ioctl, unsigned long arg)
{
- void __user *argp = (void __user *)arg;
+ struct kvm_msr_list __user *user_msr_list;
+ struct kvm_cpuid2 __user *cpuid_arg;
+ struct kvm_msr_list msr_list;
+ struct kvm_cpuid2 cpuid;
+ unsigned int n;
long r;
switch (ioctl) {
- case KVM_GET_MSR_INDEX_LIST: {
- struct kvm_msr_list __user *user_msr_list = argp;
- struct kvm_msr_list msr_list;
- unsigned n;
+ case KVM_GET_MSR_INDEX_LIST:
+ user_msr_list = (void __user *)arg;
r = -EFAULT;
if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
@@ -3441,11 +3443,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
goto out;
r = 0;
break;
- }
case KVM_GET_SUPPORTED_CPUID:
- case KVM_GET_EMULATED_CPUID: {
- struct kvm_cpuid2 __user *cpuid_arg = argp;
- struct kvm_cpuid2 cpuid;
+ case KVM_GET_EMULATED_CPUID:
+ cpuid_arg = (void __user *)arg;
r = -EFAULT;
if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
@@ -3461,18 +3461,15 @@ long kvm_arch_dev_ioctl(struct file *filp,
goto out;
r = 0;
break;
- }
- case KVM_X86_GET_MCE_CAP_SUPPORTED: {
+ case KVM_X86_GET_MCE_CAP_SUPPORTED:
r = -EFAULT;
- if (copy_to_user(argp, &kvm_mce_cap_supported,
+ if (copy_to_user((void __user *)arg, &kvm_mce_cap_supported,
sizeof(kvm_mce_cap_supported)))
goto out;
r = 0;
break;
- case KVM_GET_MSR_FEATURE_INDEX_LIST: {
- struct kvm_msr_list __user *user_msr_list = argp;
- struct kvm_msr_list msr_list;
- unsigned int n;
+ case KVM_GET_MSR_FEATURE_INDEX_LIST:
+ user_msr_list = (void __user *)arg;
r = -EFAULT;
if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
@@ -3490,11 +3487,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
goto out;
r = 0;
break;
- }
case KVM_GET_MSRS:
- r = msr_io(NULL, argp, do_get_msr_feature, 1);
+ r = msr_io(NULL, (void __user *)arg, do_get_msr_feature, 1);
break;
- }
default:
r = -EINVAL;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl()
2020-02-28 5:25 [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl() Xiaoyao Li
@ 2020-02-28 15:44 ` Sean Christopherson
2020-02-29 2:42 ` Xiaoyao Li
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2020-02-28 15:44 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On Fri, Feb 28, 2020 at 01:25:27PM +0800, Xiaoyao Li wrote:
> Remove unnecessary brackets from the case statements in
They aren't unnecessary, e.g. simply taking away brackets without
refactoring the code will break the build.
> kvm_arch_dev_ioctl().
>
> The brackets are visually confusing and error-prone, e.g., brackets of
They're confusing when they're broken, but IMO they're a non-issue when
used correctly, which is the vast majority of the time. I wouldn't say I
love brackets, but for me it's preferrable to having a big pile of
variables at the top of the function. I also find having the struct type
in the case helpful, e.g. it's easy to figure out which struct corresponds
to which ioctl in the API.
And despite this being the second instance of this style of bug in KVM in
the last few months, I don't think it's fair to call brackets error-prone.
These are literally the only two times I've ever seen this class of bug.
Regardless, using brackets to create a new scope in a case statement is a
widely used pattern:
$ git grep case | grep ": {" | wc -l
1954
Eliminating all current and future uses isn't realistic.
A better way to help prevent these type of bugs from being introduced
would be to teach checkpatch to issue a warning if a set of brackets
encapsulates a case statement.
Fixing this particular bug is then a small patch, and maybe throw in an
opportunistic cleanup to add a "break" in the default path.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2103101eca78..f059697bf61e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3485,6 +3485,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
goto out;
r = 0;
break;
+ }
case KVM_GET_MSR_FEATURE_INDEX_LIST: {
struct kvm_msr_list __user *user_msr_list = argp;
struct kvm_msr_list msr_list;
@@ -3510,9 +3511,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
case KVM_GET_MSRS:
r = msr_io(NULL, argp, do_get_msr_feature, 1);
break;
- }
default:
r = -EINVAL;
+ break;
}
out:
return r;
> case KVM_X86_GET_MCE_CAP_SUPPORTED accidently includes case
> KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS.
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> arch/x86/kvm/x86.c | 33 ++++++++++++++-------------------
> 1 file changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..9efd693189df 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3412,14 +3412,16 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> long kvm_arch_dev_ioctl(struct file *filp,
> unsigned int ioctl, unsigned long arg)
> {
> - void __user *argp = (void __user *)arg;
> + struct kvm_msr_list __user *user_msr_list;
> + struct kvm_cpuid2 __user *cpuid_arg;
> + struct kvm_msr_list msr_list;
> + struct kvm_cpuid2 cpuid;
> + unsigned int n;
> long r;
>
> switch (ioctl) {
> - case KVM_GET_MSR_INDEX_LIST: {
> - struct kvm_msr_list __user *user_msr_list = argp;
> - struct kvm_msr_list msr_list;
> - unsigned n;
> + case KVM_GET_MSR_INDEX_LIST:
> + user_msr_list = (void __user *)arg;
>
> r = -EFAULT;
> if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
> @@ -3441,11 +3443,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
> goto out;
> r = 0;
> break;
> - }
> case KVM_GET_SUPPORTED_CPUID:
> - case KVM_GET_EMULATED_CPUID: {
> - struct kvm_cpuid2 __user *cpuid_arg = argp;
> - struct kvm_cpuid2 cpuid;
> + case KVM_GET_EMULATED_CPUID:
> + cpuid_arg = (void __user *)arg;
>
> r = -EFAULT;
> if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
> @@ -3461,18 +3461,15 @@ long kvm_arch_dev_ioctl(struct file *filp,
> goto out;
> r = 0;
> break;
> - }
> - case KVM_X86_GET_MCE_CAP_SUPPORTED: {
> + case KVM_X86_GET_MCE_CAP_SUPPORTED:
> r = -EFAULT;
> - if (copy_to_user(argp, &kvm_mce_cap_supported,
> + if (copy_to_user((void __user *)arg, &kvm_mce_cap_supported,
> sizeof(kvm_mce_cap_supported)))
> goto out;
> r = 0;
> break;
> - case KVM_GET_MSR_FEATURE_INDEX_LIST: {
> - struct kvm_msr_list __user *user_msr_list = argp;
> - struct kvm_msr_list msr_list;
> - unsigned int n;
> + case KVM_GET_MSR_FEATURE_INDEX_LIST:
> + user_msr_list = (void __user *)arg;
>
> r = -EFAULT;
> if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
> @@ -3490,11 +3487,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
> goto out;
> r = 0;
> break;
> - }
> case KVM_GET_MSRS:
> - r = msr_io(NULL, argp, do_get_msr_feature, 1);
> + r = msr_io(NULL, (void __user *)arg, do_get_msr_feature, 1);
> break;
> - }
> default:
> r = -EINVAL;
> }
> --
> 2.19.1
>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl()
2020-02-28 15:44 ` Sean Christopherson
@ 2020-02-29 2:42 ` Xiaoyao Li
0 siblings, 0 replies; 3+ messages in thread
From: Xiaoyao Li @ 2020-02-29 2:42 UTC (permalink / raw)
To: Sean Christopherson
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel
On 2/28/2020 11:44 PM, Sean Christopherson wrote:
> On Fri, Feb 28, 2020 at 01:25:27PM +0800, Xiaoyao Li wrote:
>> Remove unnecessary brackets from the case statements in
>
> They aren't unnecessary, e.g. simply taking away brackets without
> refactoring the code will break the build.
>
>> kvm_arch_dev_ioctl().
>>
>> The brackets are visually confusing and error-prone, e.g., brackets of
>
> They're confusing when they're broken, but IMO they're a non-issue when
> used correctly, which is the vast majority of the time. I wouldn't say I
> love brackets, but for me it's preferrable to having a big pile of
> variables at the top of the function. I also find having the struct type
> in the case helpful, e.g. it's easy to figure out which struct corresponds
> to which ioctl in the API.
>
> And despite this being the second instance of this style of bug in KVM in
> the last few months, I don't think it's fair to call brackets error-prone.
> These are literally the only two times I've ever seen this class of bug.
Fair enough.
> Regardless, using brackets to create a new scope in a case statement is a
> widely used pattern:
>
> $ git grep case | grep ": {" | wc -l
> 1954
>
> Eliminating all current and future uses isn't realistic.
>
> A better way to help prevent these type of bugs from being introduced
> would be to teach checkpatch to issue a warning if a set of brackets
> encapsulates a case statement.
>
> Fixing this particular bug is then a small patch, and maybe throw in an
> opportunistic cleanup to add a "break" in the default path.
OK. I'll go this way.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2103101eca78..f059697bf61e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3485,6 +3485,7 @@ long kvm_arch_dev_ioctl(struct file *filp,
> goto out;
> r = 0;
> break;
> + }
> case KVM_GET_MSR_FEATURE_INDEX_LIST: {
> struct kvm_msr_list __user *user_msr_list = argp;
> struct kvm_msr_list msr_list;
> @@ -3510,9 +3511,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
> case KVM_GET_MSRS:
> r = msr_io(NULL, argp, do_get_msr_feature, 1);
> break;
> - }
> default:
> r = -EINVAL;
> + break;
> }
> out:
> return r;
>
>> case KVM_X86_GET_MCE_CAP_SUPPORTED accidently includes case
>> KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS.
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>> arch/x86/kvm/x86.c | 33 ++++++++++++++-------------------
>> 1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index ddd1d296bd20..9efd693189df 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -3412,14 +3412,16 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> long kvm_arch_dev_ioctl(struct file *filp,
>> unsigned int ioctl, unsigned long arg)
>> {
>> - void __user *argp = (void __user *)arg;
>> + struct kvm_msr_list __user *user_msr_list;
>> + struct kvm_cpuid2 __user *cpuid_arg;
>> + struct kvm_msr_list msr_list;
>> + struct kvm_cpuid2 cpuid;
>> + unsigned int n;
>> long r;
>>
>> switch (ioctl) {
>> - case KVM_GET_MSR_INDEX_LIST: {
>> - struct kvm_msr_list __user *user_msr_list = argp;
>> - struct kvm_msr_list msr_list;
>> - unsigned n;
>> + case KVM_GET_MSR_INDEX_LIST:
>> + user_msr_list = (void __user *)arg;
>>
>> r = -EFAULT;
>> if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
>> @@ -3441,11 +3443,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>> goto out;
>> r = 0;
>> break;
>> - }
>> case KVM_GET_SUPPORTED_CPUID:
>> - case KVM_GET_EMULATED_CPUID: {
>> - struct kvm_cpuid2 __user *cpuid_arg = argp;
>> - struct kvm_cpuid2 cpuid;
>> + case KVM_GET_EMULATED_CPUID:
>> + cpuid_arg = (void __user *)arg;
>>
>> r = -EFAULT;
>> if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
>> @@ -3461,18 +3461,15 @@ long kvm_arch_dev_ioctl(struct file *filp,
>> goto out;
>> r = 0;
>> break;
>> - }
>> - case KVM_X86_GET_MCE_CAP_SUPPORTED: {
>> + case KVM_X86_GET_MCE_CAP_SUPPORTED:
>> r = -EFAULT;
>> - if (copy_to_user(argp, &kvm_mce_cap_supported,
>> + if (copy_to_user((void __user *)arg, &kvm_mce_cap_supported,
>> sizeof(kvm_mce_cap_supported)))
>> goto out;
>> r = 0;
>> break;
>> - case KVM_GET_MSR_FEATURE_INDEX_LIST: {
>> - struct kvm_msr_list __user *user_msr_list = argp;
>> - struct kvm_msr_list msr_list;
>> - unsigned int n;
>> + case KVM_GET_MSR_FEATURE_INDEX_LIST:
>> + user_msr_list = (void __user *)arg;
>>
>> r = -EFAULT;
>> if (copy_from_user(&msr_list, user_msr_list, sizeof(msr_list)))
>> @@ -3490,11 +3487,9 @@ long kvm_arch_dev_ioctl(struct file *filp,
>> goto out;
>> r = 0;
>> break;
>> - }
>> case KVM_GET_MSRS:
>> - r = msr_io(NULL, argp, do_get_msr_feature, 1);
>> + r = msr_io(NULL, (void __user *)arg, do_get_msr_feature, 1);
>> break;
>> - }
>> default:
>> r = -EINVAL;
>> }
>> --
>> 2.19.1
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-29 2:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-28 5:25 [PATCH] KVM: x86: Remove superfluous brackets in kvm_arch_dev_ioctl() Xiaoyao Li
2020-02-28 15:44 ` Sean Christopherson
2020-02-29 2:42 ` Xiaoyao Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox