Linux Virtualization - Jul 2013 - [patch v2] virtio: console: cleanup an error message

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

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 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
Rusty handles virtio stuff, please cc: him on these.

thanks,

greg k-h

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 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");
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 at redhat.com>

Rusty, please pick this up.

		Amit

Dan Carpenter <dan.carpenter at oracle.com>
writes:
> The PTR_ERR(NULL) here is not useful.
>
> Signed-off-by: Dan Carpenter <dan.carpenter at 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 at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization