Linux Virtualization - Jul 2013 - [patch] 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 <dan.carpenter at oracle.com>

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1b456fe..ad2cd6d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -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));
 	}

On (Fri) 19 Jul 2013 [08:50:49], Dan Carpenter wrote:
> 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 <dan.carpenter at oracle.com>
Reviewed-by: Amit Shah <amit.shah at redhat.com>

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

Thanks,
		Amit

On Friday 19 July 2013, Dan Carpenter wrote:
> 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 <dan.carpenter at oracle.com>
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
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1b456fe..ad2cd6d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -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));
>         }

On Fri, Jul 19, 2013 at 12:28:41PM +0200, Arnd Bergmann
wrote:
> On Friday 19 July 2013, Dan Carpenter wrote:
> > 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 <dan.carpenter at oracle.com>
> 
> 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

The PTR_ERR(NULL) here is not useful.

Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
---
v2: completely different

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 1b456fe..4cf46d8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -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);