Skip to contentSkip to conversation

virtio: console: fix error handling for debugfs_create_dir()

PATCH In review Dan Carpenter 5 participants 19 messages Jul 19, 2013 – Jul 30, 2013

Subsystems (4)
Kernel janitors Char and misc drivers virtio core virtio console driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 virtio: console: fix error handling for debugfs_create_dir()

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>
drivers/char/virtio_console.c +1 -1
index 1b456fe..ad2cd6d 100644
@@ -2215,7 +2215,7 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
+	if (IS_ERR_OR_NULL(pdrvdata.debugfs_dir)) {
 		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
 			   PTR_ERR(pdrvdata.debugfs_dir));
 	}
Reviewed-by on v1 1/1
#1 lore raw
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Amit Shah <[email protected]>

Today seems to be the 'fix virtio-console day'.

Thanks,
Amit

maintainer
#2 lore raw
In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Arnd

Quoted 13 lines
drivers/char/virtio_console.c +1 -1
index 1b456fe..ad2cd6d 100644
@@ -2215,7 +2215,7 @@ static int __init init(void)
        }
 
        pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-       if (!pdrvdata.debugfs_dir) {
+       if (IS_ERR_OR_NULL(pdrvdata.debugfs_dir)) {
                pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
                           PTR_ERR(pdrvdata.debugfs_dir));
        }
author
#3 lore raw
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Yeah. You're right. But the original code is still wrong and will oops if debugfs is disabled. We should set the pointer to NULL if we get a ERR_PTR().

I will send a v2 patch.

regards,
dan carpenter

maintainer
#4 lore raw
Quoted 13 lines · Dan Carpenter
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Quoted 5 lines

Yeah. You're right. But the original code is still wrong and will oops if debugfs is disabled. We should set the pointer to NULL if we get a ERR_PTR().

I will send a v2 patch.

I don't see where that oops would happen. In the code I'm looking at, all uses of ->debugfs_dir only ever get passed into other debugfs functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional and correct to me.

Arnd

author
#5 lore raw

PATCH v2 1/1 virtio: console: cleanup an error message

In reply to Arnd Bergmann

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +2 -4
index 1b456fe..4cf46d8 100644
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");
 	INIT_LIST_HEAD(&pdrvdata.consoles);
 	INIT_LIST_HEAD(&pdrvdata.portdevs);
maintainer
#6 lore raw

Re: [patch v2] virtio: console: cleanup an error message

drivers/char/virtio_console.c

Quoted 10 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

Rusty handles virtio stuff, please cc: him on these.

thanks,

greg k-h

Reviewed-by on v2 1/1
#7 lore raw

Re: [patch v2] virtio: console: cleanup an error message

In reply to Dan Carpenter
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

When debugfs is disabled, we'll get an error return, and not print any message.

Seems OK to me.

Reviewed-by Amit Shah <[email protected]>

Rusty, please pick this up.

Amit

maintainer
#8 lore raw

Re: [patch v2] virtio: console: cleanup an error message

Quoted 25 lines · Amit Shah
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

Quoted 2 lines

When debugfs is disabled, we'll get an error return, and not print any message.

Not true, you will still print the message if debugfs is disabled, as .debugfs_dir will not be NULL.

Why even print anything at all? It's debugfs, you shouldn't really care that much about it :)

thanks,

greg k-h

maintainer
#9 lore raw

Re: [patch v2] virtio: console: cleanup an error message

In reply to Dan Carpenter
Quoted 25 lines · Amit Shah
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

Quoted 2 lines

When debugfs is disabled, we'll get an error return, and not print any message.

Not true, you will still print the message if debugfs is disabled, as .debugfs_dir will not be NULL.

Why even print anything at all? It's debugfs, you shouldn't really care that much about it :)

thanks,

greg k-h

author
#10 lore raw

Re: [patch v2] virtio: console: cleanup an error message

Quoted 26 lines · Greg Kroah-Hartman
Quoted 25 lines · Amit Shah
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

Quoted 9 lines
Quoted 2 lines

When debugfs is disabled, we'll get an error return, and not print any message.

Not true, you will still print the message if debugfs is disabled, as .debugfs_dir will not be NULL.

Why even print anything at all? It's debugfs, you shouldn't really care that much about it :)

Yes yes. That's what this code does. It only checks for NULL. The debugfs was a little confusing at first but I get it now.

regards,
dan carpenter

maintainer
#11 lore raw

Re: [patch v2] virtio: console: cleanup an error message

Quoted 33 lines · Dan Carpenter
Quoted 32 lines · Greg Kroah-Hartman
Quoted 25 lines · Amit Shah
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

Quoted 2 lines

When debugfs is disabled, we'll get an error return, and not print any message.

Not true, you will still print the message if debugfs is disabled, as .debugfs_dir will not be NULL.

Quoted 6 lines
Quoted 3 lines

Why even print anything at all? It's debugfs, you shouldn't really care that much about it :)

Yes yes. That's what this code does. It only checks for NULL. The debugfs was a little confusing at first but I get it now.

Ugh, you are right, that's what I get for writing email before my morning coffee...

maintainer
#12 lore raw

Re: [patch v2] virtio: console: cleanup an error message

In reply to Dan Carpenter
Quoted 33 lines · Dan Carpenter
Quoted 32 lines · Greg Kroah-Hartman
Quoted 25 lines · Amit Shah
Quoted 11 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

drivers/char/virtio_console.c +0 -0
index 1b456fe..4cf46d8 100644

drivers/char/virtio_console.c static int __init init(void) · line 2219

Quoted 9 lines
drivers/char/virtio_console.c +2 -4
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");

When debugfs is enabled and creating the dir fails, we'll print this warning message.

Quoted 2 lines

When debugfs is disabled, we'll get an error return, and not print any message.

Not true, you will still print the message if debugfs is disabled, as .debugfs_dir will not be NULL.

Quoted 6 lines
Quoted 3 lines

Why even print anything at all? It's debugfs, you shouldn't really care that much about it :)

Yes yes. That's what this code does. It only checks for NULL. The debugfs was a little confusing at first but I get it now.

Ugh, you are right, that's what I get for writing email before my morning coffee...

Applied
#13 lore raw

Re: [patch v2] virtio: console: cleanup an error message

In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

The PTR_ERR(NULL) here is not useful.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: completely different

Applied.

Thanks,
Rusty.

Quoted 21 lines
drivers/char/virtio_console.c +2 -4
index 1b456fe..4cf46d8 100644
@@ -2215,10 +2215,8 @@ static int __init init(void)
 	}
 
 	pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-	if (!pdrvdata.debugfs_dir) {
-		pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
-			   PTR_ERR(pdrvdata.debugfs_dir));
-	}
+	if (!pdrvdata.debugfs_dir)
+		pr_warning("Error creating debugfs dir for virtio-ports\n");
 	INIT_LIST_HEAD(&pdrvdata.consoles);
 	INIT_LIST_HEAD(&pdrvdata.portdevs);

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

maintainer
#14 lore raw
In reply to Dan Carpenter
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Arnd

Quoted 13 lines
drivers/char/virtio_console.c +1 -1
index 1b456fe..ad2cd6d 100644
@@ -2215,7 +2215,7 @@ static int __init init(void)
        }
 
        pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
-       if (!pdrvdata.debugfs_dir) {
+       if (IS_ERR_OR_NULL(pdrvdata.debugfs_dir)) {
                pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
                           PTR_ERR(pdrvdata.debugfs_dir));
        }
maintainer
#15 lore raw
In reply to Dan Carpenter
Quoted 13 lines · Dan Carpenter
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Quoted 5 lines

Yeah. You're right. But the original code is still wrong and will oops if debugfs is disabled. We should set the pointer to NULL if we get a ERR_PTR().

I will send a v2 patch.

I don't see where that oops would happen. In the code I'm looking at, all uses of ->debugfs_dir only ever get passed into other debugfs functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional and correct to me.

Arnd

maintainer
#16 lore raw
Quoted 18 lines · Arnd Bergmann
Quoted 17 lines · Dan Carpenter
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Yeah. You're right. But the original code is still wrong and will oops if debugfs is disabled. We should set the pointer to NULL if we get a ERR_PTR().

Quoted 8 lines
Quoted 1 line

I will send a v2 patch.

I don't see where that oops would happen. In the code I'm looking at, all uses of ->debugfs_dir only ever get passed into other debugfs functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional and correct to me.

It was the best interface design I could create, making it very easy for drivers to use and not really worry at all if debugfs was failing or not, or if it was even present in the system or not. That was the design goal I had for it when I wrote it.

thanks,

greg k-h

author
#17 lore raw
In reply to Arnd Bergmann
Quoted 6 lines

I don't see where that oops would happen. In the code I'm looking at, all uses of ->debugfs_dir only ever get passed into other debugfs functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional and correct to me.

Ah. Ok. You're right.

regards,
dan carpenter

maintainer
#18 lore raw
In reply to Dan Carpenter
Quoted 18 lines · Arnd Bergmann
Quoted 17 lines · Dan Carpenter
Quoted 11 lines · Arnd Bergmann
Quoted 5 lines · Dan Carpenter

debugfs_create_dir() returns ERR_PTR(-ENODEV) if debugfs is disabled. Also my static checker doesn't like it when we print the error code, but it's always just NULL.

Signed-off-by Dan Carpenter <[email protected]>

This looks wrong. debugfs_create_dir intentionally returns non-NULL so failing to create the directory does not trigger an error condition if debugfs is disabled.

Yeah. You're right. But the original code is still wrong and will oops if debugfs is disabled. We should set the pointer to NULL if we get a ERR_PTR().

Quoted 8 lines
Quoted 1 line

I will send a v2 patch.

I don't see where that oops would happen. In the code I'm looking at, all uses of ->debugfs_dir only ever get passed into other debugfs functions that are stubbed out to empty inline functions.

It's not the most obvious interface design, but this all seems intentional and correct to me.

It was the best interface design I could create, making it very easy for drivers to use and not really worry at all if debugfs was failing or not, or if it was even present in the system or not. That was the design goal I had for it when I wrote it.

thanks,

greg k-h