When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Jason Baron <[email protected]>
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
lib/dynamic_debug.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8a16c2d498e9..c60409138e13 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -993,20 +993,14 @@ static __initdata int ddebug_init_success;
static int __init dynamic_debug_init_debugfs(void)
{
- struct dentry *dir, *file;
+ struct dentry *dir;
if (!ddebug_init_success)
return -ENODEV;
dir = debugfs_create_dir("dynamic_debug", NULL);
- if (!dir)
- return -ENOMEM;
- file = debugfs_create_file("control", 0644, dir, NULL,
- &ddebug_proc_fops);
- if (!file) {
- debugfs_remove(dir);
- return -ENOMEM;
- }
+ debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
+
return 0;
}
--
2.22.0
On 6/12/19 11:35 AM, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Cc: Jason Baron <[email protected]>
> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> lib/dynamic_debug.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 8a16c2d498e9..c60409138e13 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -993,20 +993,14 @@ static __initdata int ddebug_init_success;
>
> static int __init dynamic_debug_init_debugfs(void)
> {
> - struct dentry *dir, *file;
> + struct dentry *dir;
>
> if (!ddebug_init_success)
> return -ENODEV;
>
> dir = debugfs_create_dir("dynamic_debug", NULL);
> - if (!dir)
> - return -ENOMEM;
> - file = debugfs_create_file("control", 0644, dir, NULL,
> - &ddebug_proc_fops);
> - if (!file) {
> - debugfs_remove(dir);
> - return -ENOMEM;
> - }
> + debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> +
> return 0;
> }
>
>
Looks like debugfs_create_dir() can return NULL, and in that case if its
passed to debugfs_create_file() then the 'control' file ends up in the
root of debugfs? I think its better to just not create the file then
have it in the wrong place so maybe the file creation should be guarded
by if(IS_ERR_OR_NULL(dir)).
Thanks,
-Jason
On Thu, Jun 13, 2019 at 10:33:23AM -0400, Jason Baron wrote:
> On 6/12/19 11:35 AM, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value. The function can work or not, but the code logic should
> > never do something different based on this.
> >
> > Cc: Jason Baron <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > lib/dynamic_debug.c | 12 +++---------
> > 1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> > index 8a16c2d498e9..c60409138e13 100644
> > --- a/lib/dynamic_debug.c
> > +++ b/lib/dynamic_debug.c
> > @@ -993,20 +993,14 @@ static __initdata int ddebug_init_success;
> >
> > static int __init dynamic_debug_init_debugfs(void)
> > {
> > - struct dentry *dir, *file;
> > + struct dentry *dir;
> >
> > if (!ddebug_init_success)
> > return -ENODEV;
> >
> > dir = debugfs_create_dir("dynamic_debug", NULL);
> > - if (!dir)
> > - return -ENOMEM;
> > - file = debugfs_create_file("control", 0644, dir, NULL,
> > - &ddebug_proc_fops);
> > - if (!file) {
> > - debugfs_remove(dir);
> > - return -ENOMEM;
> > - }
> > + debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> > +
> > return 0;
> > }
> >
> >
>
> Looks like debugfs_create_dir() can return NULL,
No it can not.
> and in that case if its passed to debugfs_create_file() then the
> 'control' file ends up in the root of debugfs?
If it could, yes, that is what would happen.
> I think its better to just not create the file then have it in the
> wrong place so maybe the file creation should be guarded by
> if(IS_ERR_OR_NULL(dir)).
As debugfs_create_dir() can not return NULL, you don't have to worry
about this :)
thanks,
greg k-h
On 6/13/19 11:59 AM, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 10:33:23AM -0400, Jason Baron wrote:
>> On 6/12/19 11:35 AM, Greg Kroah-Hartman wrote:
>>> When calling debugfs functions, there is no need to ever check the
>>> return value. The function can work or not, but the code logic should
>>> never do something different based on this.
>>>
>>> Cc: Jason Baron <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
>>> ---
>>> lib/dynamic_debug.c | 12 +++---------
>>> 1 file changed, 3 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
>>> index 8a16c2d498e9..c60409138e13 100644
>>> --- a/lib/dynamic_debug.c
>>> +++ b/lib/dynamic_debug.c
>>> @@ -993,20 +993,14 @@ static __initdata int ddebug_init_success;
>>>
>>> static int __init dynamic_debug_init_debugfs(void)
>>> {
>>> - struct dentry *dir, *file;
>>> + struct dentry *dir;
>>>
>>> if (!ddebug_init_success)
>>> return -ENODEV;
>>>
>>> dir = debugfs_create_dir("dynamic_debug", NULL);
>>> - if (!dir)
>>> - return -ENOMEM;
>>> - file = debugfs_create_file("control", 0644, dir, NULL,
>>> - &ddebug_proc_fops);
>>> - if (!file) {
>>> - debugfs_remove(dir);
>>> - return -ENOMEM;
>>> - }
>>> + debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
>>> +
>>> return 0;
>>> }
>>>
>>>
>>
>> Looks like debugfs_create_dir() can return NULL,
>
> No it can not.
>
Ok, I looked at the wrong definition for failed_creating() :(
In that case:
Acked-by: Jason Baron <[email protected]>
Thanks,
-Jason
On Thu, Jun 13, 2019 at 12:09:00PM -0400, Jason Baron wrote:
>
>
> On 6/13/19 11:59 AM, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 10:33:23AM -0400, Jason Baron wrote:
> >> On 6/12/19 11:35 AM, Greg Kroah-Hartman wrote:
> >>> When calling debugfs functions, there is no need to ever check the
> >>> return value. The function can work or not, but the code logic should
> >>> never do something different based on this.
> >>>
> >>> Cc: Jason Baron <[email protected]>
> >>> Cc: [email protected]
> >>> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> >>> ---
> >>> lib/dynamic_debug.c | 12 +++---------
> >>> 1 file changed, 3 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> >>> index 8a16c2d498e9..c60409138e13 100644
> >>> --- a/lib/dynamic_debug.c
> >>> +++ b/lib/dynamic_debug.c
> >>> @@ -993,20 +993,14 @@ static __initdata int ddebug_init_success;
> >>>
> >>> static int __init dynamic_debug_init_debugfs(void)
> >>> {
> >>> - struct dentry *dir, *file;
> >>> + struct dentry *dir;
> >>>
> >>> if (!ddebug_init_success)
> >>> return -ENODEV;
> >>>
> >>> dir = debugfs_create_dir("dynamic_debug", NULL);
> >>> - if (!dir)
> >>> - return -ENOMEM;
> >>> - file = debugfs_create_file("control", 0644, dir, NULL,
> >>> - &ddebug_proc_fops);
> >>> - if (!file) {
> >>> - debugfs_remove(dir);
> >>> - return -ENOMEM;
> >>> - }
> >>> + debugfs_create_file("control", 0644, dir, NULL, &ddebug_proc_fops);
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>>
> >>
> >> Looks like debugfs_create_dir() can return NULL,
> >
> > No it can not.
> >
>
> Ok, I looked at the wrong definition for failed_creating() :(
Yeah, tracefs should also probably change, it makes users of their api
easier...
> In that case:
>
> Acked-by: Jason Baron <[email protected]>
Wonderful, thanks for the review.
greg k-h