* [patch] virtio: console: fix error handling for debugfs_create_dir() @ 2013-07-19 5:50 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-19 5:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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)); } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [patch] virtio: console: fix error handling for debugfs_create_dir() @ 2013-07-19 5:50 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-19 5:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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)); } ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 5:50 ` Dan Carpenter @ 2013-07-19 6:25 ` Amit Shah -1 siblings, 0 replies; 28+ messages in thread From: Amit Shah @ 2013-07-19 6:13 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() @ 2013-07-19 6:25 ` Amit Shah 0 siblings, 0 replies; 28+ messages in thread From: Amit Shah @ 2013-07-19 6:25 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 5:50 ` Dan Carpenter (?) (?) @ 2013-07-19 10:28 ` Arnd Bergmann 2013-07-20 21:50 ` Dan Carpenter 2013-07-22 20:41 ` Dan Carpenter -1 siblings, 2 replies; 28+ messages in thread From: Arnd Bergmann @ 2013-07-19 10:28 UTC (permalink / raw) To: kernel-janitors 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)); > } ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 10:28 ` Arnd Bergmann @ 2013-07-20 21:50 ` Dan Carpenter 2013-07-22 20:41 ` Dan Carpenter 1 sibling, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-20 21:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() @ 2013-07-20 21:50 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-20 21:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-20 21:50 ` Dan Carpenter (?) @ 2013-07-21 9:36 ` Arnd Bergmann -1 siblings, 0 replies; 28+ messages in thread From: Arnd Bergmann @ 2013-07-21 9:36 UTC (permalink / raw) To: Dan Carpenter Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [patch v2] virtio: console: cleanup an error message 2013-07-19 10:28 ` Arnd Bergmann @ 2013-07-22 20:41 ` Dan Carpenter 2013-07-22 20:41 ` Dan Carpenter 1 sibling, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-22 20:41 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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); ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [patch v2] virtio: console: cleanup an error message @ 2013-07-22 20:41 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-22 20:41 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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); ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-22 20:41 ` Dan Carpenter @ 2013-07-23 4:35 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-23 4:35 UTC (permalink / raw) To: Dan Carpenter; +Cc: Amit Shah, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message @ 2013-07-23 4:35 ` Greg Kroah-Hartman 0 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-23 4:35 UTC (permalink / raw) To: Dan Carpenter; +Cc: Amit Shah, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-22 20:41 ` Dan Carpenter @ 2013-07-29 7:23 ` Amit Shah -1 siblings, 0 replies; 28+ messages in thread From: Amit Shah @ 2013-07-29 7:11 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message @ 2013-07-29 7:23 ` Amit Shah 0 siblings, 0 replies; 28+ messages in thread From: Amit Shah @ 2013-07-29 7:23 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Kroah-Hartman, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-29 7:23 ` Amit Shah (?) @ 2013-07-29 13:12 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-29 13:12 UTC (permalink / raw) To: Amit Shah; +Cc: kernel-janitors, Arnd Bergmann, Dan Carpenter, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-22 20:41 ` Dan Carpenter ` (2 preceding siblings ...) (?) @ 2013-07-29 13:12 ` Greg Kroah-Hartman 2013-07-29 13:35 ` Dan Carpenter -1 siblings, 1 reply; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-29 13:12 UTC (permalink / raw) To: kernel-janitors 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-29 13:12 ` Greg Kroah-Hartman @ 2013-07-29 13:35 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-29 13:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Amit Shah, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message @ 2013-07-29 13:35 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-29 13:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Amit Shah, kernel-janitors, Arnd Bergmann, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-29 13:35 ` Dan Carpenter (?) @ 2013-07-29 13:44 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-29 13:44 UTC (permalink / raw) To: Dan Carpenter; +Cc: Amit Shah, kernel-janitors, Arnd Bergmann, virtualization 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-22 20:41 ` Dan Carpenter ` (3 preceding siblings ...) (?) @ 2013-07-29 13:44 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-29 13:44 UTC (permalink / raw) To: kernel-janitors 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... ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message 2013-07-22 20:41 ` Dan Carpenter @ 2013-07-30 6:35 ` Rusty Russell -1 siblings, 0 replies; 28+ messages in thread From: Rusty Russell @ 2013-07-30 6:23 UTC (permalink / raw) To: Dan Carpenter, Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch v2] virtio: console: cleanup an error message @ 2013-07-30 6:35 ` Rusty Russell 0 siblings, 0 replies; 28+ messages in thread From: Rusty Russell @ 2013-07-30 6:35 UTC (permalink / raw) To: Dan Carpenter, Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 5:50 ` Dan Carpenter ` (2 preceding siblings ...) (?) @ 2013-07-19 10:28 ` Arnd Bergmann -1 siblings, 0 replies; 28+ messages in thread From: Arnd Bergmann @ 2013-07-19 10:28 UTC (permalink / raw) To: Dan Carpenter Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization 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)); > } ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 5:50 ` Dan Carpenter ` (3 preceding siblings ...) (?) @ 2013-07-21 9:36 ` Arnd Bergmann 2013-07-21 15:49 ` Greg Kroah-Hartman 2013-07-21 22:33 ` Dan Carpenter -1 siblings, 2 replies; 28+ messages in thread From: Arnd Bergmann @ 2013-07-21 9:36 UTC (permalink / raw) To: kernel-janitors 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-21 9:36 ` Arnd Bergmann @ 2013-07-21 15:49 ` Greg Kroah-Hartman 2013-07-21 22:33 ` Dan Carpenter 1 sibling, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-21 15:49 UTC (permalink / raw) To: Arnd Bergmann; +Cc: Amit Shah, kernel-janitors, Dan Carpenter, virtualization 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-21 9:36 ` Arnd Bergmann @ 2013-07-21 22:33 ` Dan Carpenter 2013-07-21 22:33 ` Dan Carpenter 1 sibling, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-21 22:33 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization > 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() @ 2013-07-21 22:33 ` Dan Carpenter 0 siblings, 0 replies; 28+ messages in thread From: Dan Carpenter @ 2013-07-21 22:33 UTC (permalink / raw) To: Arnd Bergmann Cc: Amit Shah, Greg Kroah-Hartman, kernel-janitors, virtualization > 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [patch] virtio: console: fix error handling for debugfs_create_dir() 2013-07-19 5:50 ` Dan Carpenter ` (4 preceding siblings ...) (?) @ 2013-07-21 15:49 ` Greg Kroah-Hartman -1 siblings, 0 replies; 28+ messages in thread From: Greg Kroah-Hartman @ 2013-07-21 15:49 UTC (permalink / raw) To: kernel-janitors 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2013-07-30 6:35 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-19 5:50 [patch] virtio: console: fix error handling for debugfs_create_dir() Dan Carpenter 2013-07-19 5:50 ` Dan Carpenter 2013-07-19 6:13 ` Amit Shah 2013-07-19 6:25 ` Amit Shah 2013-07-19 10:28 ` Arnd Bergmann 2013-07-20 21:50 ` Dan Carpenter 2013-07-20 21:50 ` Dan Carpenter 2013-07-21 9:36 ` Arnd Bergmann 2013-07-22 20:41 ` [patch v2] virtio: console: cleanup an error message Dan Carpenter 2013-07-22 20:41 ` Dan Carpenter 2013-07-23 4:35 ` Greg Kroah-Hartman 2013-07-23 4:35 ` Greg Kroah-Hartman 2013-07-29 7:11 ` Amit Shah 2013-07-29 7:23 ` Amit Shah 2013-07-29 13:12 ` Greg Kroah-Hartman 2013-07-29 13:12 ` Greg Kroah-Hartman 2013-07-29 13:35 ` Dan Carpenter 2013-07-29 13:35 ` Dan Carpenter 2013-07-29 13:44 ` Greg Kroah-Hartman 2013-07-29 13:44 ` Greg Kroah-Hartman 2013-07-30 6:23 ` Rusty Russell 2013-07-30 6:35 ` Rusty Russell 2013-07-19 10:28 ` [patch] virtio: console: fix error handling for debugfs_create_dir() Arnd Bergmann 2013-07-21 9:36 ` Arnd Bergmann 2013-07-21 15:49 ` Greg Kroah-Hartman 2013-07-21 22:33 ` Dan Carpenter 2013-07-21 22:33 ` Dan Carpenter 2013-07-21 15:49 ` Greg Kroah-Hartman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.