[PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

[PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: <x86@kernel.org>
Cc: <kvm@vger.kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/x86/kvm/debugfs.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
index c19c7ede9bd6..827cd58400d2 100644
--- a/arch/x86/kvm/debugfs.c
+++ b/arch/x86/kvm/debugfs.c
@@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
 
 int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-	struct dentry *ret;
-
-	ret = debugfs_create_file("tsc-offset", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_offset_fops);
-	if (!ret)
-		return -ENOMEM;
+	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
+			    &vcpu_tsc_offset_fops);
 
 	if (kvm_has_tsc_control) {
-		ret = debugfs_create_file("tsc-scaling-ratio", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_scaling_fops);
-		if (!ret)
-			return -ENOMEM;
-		ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
-							vcpu->debugfs_dentry,
-							vcpu, &vcpu_tsc_scaling_frac_fops);
-		if (!ret)
-			return -ENOMEM;
-
+		debugfs_create_file("tsc-scaling-ratio", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_fops);
+		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
+				    vcpu->debugfs_dentry, vcpu,
+				    &vcpu_tsc_scaling_frac_fops);
 	}
 
 	return 0;
-- 
2.20.1

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

[Paolo Bonzini]

On 22/01/19 15:35, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: <x86@kernel.org>
> Cc: <kvm@vger.kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  arch/x86/kvm/debugfs.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
> index c19c7ede9bd6..827cd58400d2 100644
> --- a/arch/x86/kvm/debugfs.c
> +++ b/arch/x86/kvm/debugfs.c
> @@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
>  
>  int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
>  {
> -	struct dentry *ret;
> -
> -	ret = debugfs_create_file("tsc-offset", 0444,
> -							vcpu->debugfs_dentry,
> -							vcpu, &vcpu_tsc_offset_fops);
> -	if (!ret)
> -		return -ENOMEM;
> +	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
> +			    &vcpu_tsc_offset_fops);
>  
>  	if (kvm_has_tsc_control) {
> -		ret = debugfs_create_file("tsc-scaling-ratio", 0444,
> -							vcpu->debugfs_dentry,
> -							vcpu, &vcpu_tsc_scaling_fops);
> -		if (!ret)
> -			return -ENOMEM;
> -		ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> -							vcpu->debugfs_dentry,
> -							vcpu, &vcpu_tsc_scaling_frac_fops);
> -		if (!ret)
> -			return -ENOMEM;
> -
> +		debugfs_create_file("tsc-scaling-ratio", 0444,
> +				    vcpu->debugfs_dentry, vcpu,
> +				    &vcpu_tsc_scaling_fops);
> +		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> +				    vcpu->debugfs_dentry, vcpu,
> +				    &vcpu_tsc_scaling_frac_fops);
>  	}
>  
>  	return 0;
> 

I'm still not sure about this.  I think it's better if debugfs files are
created "all or none", i.e. something like

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5ecea812cb6a..ce70c30b2861 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2573,9 +2573,7 @@ static int kvm_vm_ioctl_create_vcpu(
 	if (r)
 		goto vcpu_destroy;

-	r = kvm_create_vcpu_debugfs(vcpu);
-	if (r)
-		goto vcpu_destroy;
+	kvm_create_vcpu_debugfs(vcpu);

 	mutex_lock(&kvm->lock);
 	if (kvm_get_vcpu_by_id(kvm, id)) {


Paolo

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Fri, Jan 25, 2019 at 06:49:47PM +0100, Paolo Bonzini wrote:
> On 22/01/19 15:35, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value.  The function can work or not, but the code logic should
> > never do something different based on this.
> > 
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Radim Krčmář" <rkrcmar@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: <x86@kernel.org>
> > Cc: <kvm@vger.kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  arch/x86/kvm/debugfs.c | 26 ++++++++------------------
> >  1 file changed, 8 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/debugfs.c b/arch/x86/kvm/debugfs.c
> > index c19c7ede9bd6..827cd58400d2 100644
> > --- a/arch/x86/kvm/debugfs.c
> > +++ b/arch/x86/kvm/debugfs.c
> > @@ -43,26 +43,16 @@ DEFINE_SIMPLE_ATTRIBUTE(vcpu_tsc_scaling_frac_fops, vcpu_get_tsc_scaling_frac_bi
> >  
> >  int kvm_arch_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
> >  {
> > -	struct dentry *ret;
> > -
> > -	ret = debugfs_create_file("tsc-offset", 0444,
> > -							vcpu->debugfs_dentry,
> > -							vcpu, &vcpu_tsc_offset_fops);
> > -	if (!ret)
> > -		return -ENOMEM;
> > +	debugfs_create_file("tsc-offset", 0444, vcpu->debugfs_dentry, vcpu,
> > +			    &vcpu_tsc_offset_fops);
> >  
> >  	if (kvm_has_tsc_control) {
> > -		ret = debugfs_create_file("tsc-scaling-ratio", 0444,
> > -							vcpu->debugfs_dentry,
> > -							vcpu, &vcpu_tsc_scaling_fops);
> > -		if (!ret)
> > -			return -ENOMEM;
> > -		ret = debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> > -							vcpu->debugfs_dentry,
> > -							vcpu, &vcpu_tsc_scaling_frac_fops);
> > -		if (!ret)
> > -			return -ENOMEM;
> > -
> > +		debugfs_create_file("tsc-scaling-ratio", 0444,
> > +				    vcpu->debugfs_dentry, vcpu,
> > +				    &vcpu_tsc_scaling_fops);
> > +		debugfs_create_file("tsc-scaling-ratio-frac-bits", 0444,
> > +				    vcpu->debugfs_dentry, vcpu,
> > +				    &vcpu_tsc_scaling_frac_fops);
> >  	}
> >  
> >  	return 0;
> > 
> 
> I'm still not sure about this.  I think it's better if debugfs files are
> created "all or none", i.e. something like
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5ecea812cb6a..ce70c30b2861 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2573,9 +2573,7 @@ static int kvm_vm_ioctl_create_vcpu(
>  	if (r)
>  		goto vcpu_destroy;
> 
> -	r = kvm_create_vcpu_debugfs(vcpu);
> -	if (r)
> -		goto vcpu_destroy;
> +	kvm_create_vcpu_debugfs(vcpu);

Ah, yes, sorry, I missed that.  I'll respin the patch with this change
in it in a few days.

thanks for the review.

greg k-h

Re: [PATCH 3/6] x86: kvm: no need to check return value of debugfs_create functions

[Paolo Bonzini]

On 26/01/19 14:53, Greg Kroah-Hartman wrote:
> Ah, yes, sorry, I missed that.  I'll respin the patch with this change
> in it in a few days.

Just drop this patch.  I'll post the kvm_vm_ioctl_create_vcpu patch as a
replacement for this one and your other debugfs_create patch for virt/kvm.

Thanks,

Paolo