Re: [PATCH] kvm: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Tue, Jan 22, 2019 at 09:39:24PM +0100, Christian Borntraeger wrote:
> 
> 
> On 22.01.2019 16:21, 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: kvm@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  virt/kvm/kvm_main.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 5ecea812cb6a..4f96450ecdfc 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2528,9 +2528,7 @@ static int kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
> > 
> >  	snprintf(dir_name, sizeof(dir_name), "vcpu%d", vcpu->vcpu_id);
> >  	vcpu->debugfs_dentry = debugfs_create_dir(dir_name,
> > -								vcpu->kvm->debugfs_dentry);
> > -	if (!vcpu->debugfs_dentry)
> > -		return -ENOMEM;
> > +						  vcpu->kvm->debugfs_dentry);
> > 
> >  	ret = kvm_arch_create_vcpu_debugfs(vcpu);
> >  	if (ret < 0) {
> > 
> 
> 
> The interesting part of these debugfs entries is that they export an interface that is used
> by the kvm_stat tool. (and all distributions that I checked have debugfs enabled).
> 
> I think it is pretty unlikely that things will fail, but the question is: do we want to reject
> VM creation if that VM cannot be observed by instrumentation or not? No idea.

No you should not as any other part of the kernel can randomly create
the same debugfs files and keep your code from being able to create
them :)

> This also brings the question: shall we move these counters out of debugfs into something else?

If you have code that relies on debugfs, yes, you need to move that out
of debugfs because more and more systems are trying to disable it due to
the obvious problems with it (i.e. leaking tons of debugging
information).

debugfs is for DEBUG information, not for "statistics about how my VM is
working".  That sounds like something you need to rely on, so debugfs is
not the place for it.

thanks,

greg k-h