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

[Michael Ellerman]

Greg Kroah-Hartman <gre...@linuxfoundation.org> writes:
> 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.

Except it does need to do something different, if the file was created
it needs to be removed in the remove path.

> diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
> index bfe4f106cffc..8e4791c6f2af 100644
> --- a/arch/powerpc/kvm/timing.c
> +++ b/arch/powerpc/kvm/timing.c
> @@ -207,19 +207,12 @@ static const struct file_operations 
> kvmppc_exit_timing_fops = {
>  void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
>  {
>       static char dbg_fname[50];
> -     struct dentry *debugfs_file;
>  
>       snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
>                current->pid, id);
> -     debugfs_file = debugfs_create_file(dbg_fname, 0666,
> -                                     kvm_debugfs_dir, vcpu,
> -                                     &kvmppc_exit_timing_fops);
> -
> -     if (!debugfs_file) {
> -             printk(KERN_ERR"%s: error creating debugfs file %s\n",
> -                     __func__, dbg_fname);
> -             return;
> -     }
> +     debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
> +                         &kvmppc_exit_timing_fops);
> +
>  
>       vcpu->arch.debugfs_exit_timing = debugfs_file;
>  }

This doesn't build:

    arch/powerpc/kvm/timing.c:217:35: error: 'debugfs_file' undeclared (first 
use in this function); did you mean 'debugfs_file_put'?

We can't just drop the assignment, we need the dentry to do the removal:

void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
{
        if (vcpu->arch.debugfs_exit_timing) {
                debugfs_remove(vcpu->arch.debugfs_exit_timing);
                vcpu->arch.debugfs_exit_timing = NULL;
        }
}


I squashed this in, which seems to work:

diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
index 8e4791c6f2af..5b7a66f86bd5 100644
--- a/arch/powerpc/kvm/timing.c
+++ b/arch/powerpc/kvm/timing.c
@@ -207,19 +207,19 @@ static const struct file_operations 
kvmppc_exit_timing_fops = {
 void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
 {
        static char dbg_fname[50];
+       struct dentry *debugfs_file;
 
        snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
                 current->pid, id);
-       debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
-                           &kvmppc_exit_timing_fops);
-
+       debugfs_file = debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir,
+                                          vcpu, &kvmppc_exit_timing_fops);
 
        vcpu->arch.debugfs_exit_timing = debugfs_file;
 }
 
 void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-       if (vcpu->arch.debugfs_exit_timing) {
+       if (!IS_ERR_OR_NULL(vcpu->arch.debugfs_exit_timing)) {
                debugfs_remove(vcpu->arch.debugfs_exit_timing);
                vcpu->arch.debugfs_exit_timing = NULL;
        }


cheers