[patch] virtio: console: fix error handling for debugfs_create_dir()

[patch] virtio: console: fix error handling for debugfs_create_dir()

[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 <dan.carpenter@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));
 	}

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Amit Shah]

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@oracle.com>

Reviewed-by: Amit Shah <amit.shah@redhat.com>

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

Thanks,
		Amit

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Arnd Bergmann]

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@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));
>         }

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Dan Carpenter]

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@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

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Arnd Bergmann]

On Saturday 20 July 2013, Dan Carpenter wrote:
> 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@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.

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

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Greg Kroah-Hartman]

On Sun, Jul 21, 2013 at 11:36:25AM +0200, Arnd Bergmann wrote:
> On Saturday 20 July 2013, Dan Carpenter wrote:
> > 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@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.
> 
> 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

Re: [patch] virtio: console: fix error handling for debugfs_create_dir()

[Dan Carpenter]

> 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

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

[Dan Carpenter]

The PTR_ERR(NULL) here is not useful.

Signed-off-by: Dan Carpenter <dan.carpenter@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);

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

[Greg Kroah-Hartman]

On Mon, Jul 22, 2013 at 11:41:00PM +0300, Dan Carpenter wrote:
> The PTR_ERR(NULL) here is not useful.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@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

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

thanks,

greg k-h

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

[Amit Shah]

On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> The PTR_ERR(NULL) here is not useful.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@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");

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 <amit.shah@redhat.com>

Rusty, please pick this up.

		Amit

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

[Greg Kroah-Hartman]

On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > The PTR_ERR(NULL) here is not useful.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@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");
> 
> 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.

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

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

[Dan Carpenter]

On Mon, Jul 29, 2013 at 06:12:31AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> > On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > > The PTR_ERR(NULL) here is not useful.
> > > 
> > > Signed-off-by: Dan Carpenter <dan.carpenter@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");
> > 
> > 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.
> 
> 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

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

[Greg Kroah-Hartman]

On Mon, Jul 29, 2013 at 04:35:38PM +0300, Dan Carpenter wrote:
> On Mon, Jul 29, 2013 at 06:12:31AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> > > On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > > > The PTR_ERR(NULL) here is not useful.
> > > > 
> > > > Signed-off-by: Dan Carpenter <dan.carpenter@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");
> > > 
> > > 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.
> > 
> > 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.

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

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

[Rusty Russell]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> The PTR_ERR(NULL) here is not useful.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: completely different

Applied.

Thanks,
Rusty.

> 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);
>  
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization