Re: [PATCH 3.12-rc7] KVM: Fix modprobe failure for kvm_intel/kvm_amd

[Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On 10/30/2013 01:03 AM, Linus Torvalds wrote:
> On Tue, Oct 29, 2013 at 12:27 PM, Raghavendra K T
> <raghavendra.kt@linux.vnet.ibm.com> wrote:
>>
>> Could one solution be cascading actual error
>> that is lost in fs/debugfs/inode.c:__create_file(), so that we could
>> take correct action in case of failure of debugfs_create_dir()?
>>
>> (ugly side is we increase total number of params for __create_file to
>> 6). or I hope there could be some better solution.
>
> The solution to this would be to simply return an error-pointer. See
> <linux/err.h>. That's what we do for most complex subsystems that
> return a pointer to a struct: rather than returning "NULL" as an
> error, return the actual error number encoded in the pointer itself.

Thank you Linus. Yes, this would have been ideal.

>
> But that would require every user of debugfs_create_dir() to be
> updated to check errors using IS_ERR() instead of checking against
> NULL, and there's quite a few of them.
>
> So I think just making the error be EEXIST is a simpler solution right now.
>

Agree.
I had below patch, and just before sending as formal mail I saw
Paolo's pull request which already took care of it.
---8<---



[-- Attachment #2: 0001-Return-EEXIST-on-debugfs_create_dir-failure-in-kvm.patch --]
[-- Type: text/x-patch, Size: 1795 bytes --]

>From ac5d9f038c273f27bea7a54aab6af79b57f56317 Mon Sep 17 00:00:00 2001
From: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
Date: Wed, 30 Oct 2013 18:59:46 +0530
Subject: [PATCH] Return EEXIST on debugfs_create_dir failure in kvm

As quoted by Linus,  EFAULT means "user passed in an invalid
virtual address pointer", which is why the error string is Bad address.
But when a debugfs directory creation fails, the above error is not valid.

Signed-off-by: Raghavendra K T <raghavendra.kt@linux.vnet.ibm.com>
---
 I understand that Tim's patch that renames directory to something like
 kvm-pv would solve kvm-amd/kvm-intel modules insertion problem. 
 This patch is to address error code  change complained by Linus.

 arch/x86/kernel/kvm.c | 2 +-
 virt/kvm/kvm_main.c   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index a0e2a8a..e475fdb 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -622,7 +622,7 @@ static int __init kvm_spinlock_debugfs(void)
 
 	d_kvm = kvm_init_debugfs();
 	if (d_kvm == NULL)
-		return -ENOMEM;
+		return -EEXIST;
 
 	d_spin_debug = debugfs_create_dir("spinlocks", d_kvm);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a9dd682..0430853 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3091,13 +3091,14 @@ static const struct file_operations *stat_fops[] = {
 
 static int kvm_init_debug(void)
 {
-	int r = -EFAULT;
+	int r = -EEXIST;
 	struct kvm_stats_debugfs_item *p;
 
 	kvm_debugfs_dir = debugfs_create_dir("kvm", NULL);
 	if (kvm_debugfs_dir == NULL)
 		goto out;
 
+	r = -EFAULT;
 	for (p = debugfs_entries; p->name; ++p) {
 		p->dentry = debugfs_create_file(p->name, 0444, kvm_debugfs_dir,
 						(void *)(long)p->offset,
-- 
1.7.11.7