diff mbox series

[2/2] scsi: scsi_debug: delete some bogus error checking

Message ID f96d6366-9271-4020-ab66-f75737a1e8bd@moroto.mountain (mailing list archive)
State Superseded
Headers show
Series [1/2] scsi: scsi_debug: fix some bugs in sdebug_error_write() | expand

Commit Message

Dan Carpenter Oct. 20, 2023, 2:15 p.m. UTC
Smatch complains that "dentry" is never initialized.  These days everyone
initializes all their stack variables to zero so this means that it will
trigger a warning every time this function is run.

Really debugfs functions are not supposed to be checked for errors so
this checking can just be deleted.

Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
See my blog for more information on the history of debugfs error
checking:

https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
---
 drivers/scsi/scsi_debug.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Wenchao Hao Oct. 20, 2023, 5:28 p.m. UTC | #1
On 2023/10/20 22:15, Dan Carpenter wrote:
> Smatch complains that "dentry" is never initialized.  These days everyone
> initializes all their stack variables to zero so this means that it will
> trigger a warning every time this function is run.
> 
> Really debugfs functions are not supposed to be checked for errors so
> this checking can just be deleted.
> 
> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> See my blog for more information on the history of debugfs error
> checking:
> 
> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> ---
>  drivers/scsi/scsi_debug.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 0a4e41d84df8..c0be9a53ac79 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -1127,7 +1127,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
>  static int sdebug_target_alloc(struct scsi_target *starget)
>  {
>  	struct sdebug_target_info *targetip;
> -	struct dentry *dentry;
>  
>  	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
>  	if (!targetip)
> @@ -1135,15 +1134,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
>  
>  	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
>  				sdebug_debugfs_root);
> -	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
> -		pr_info("%s: failed to create debugfs directory for target %s\n",
> -			__func__, dev_name(&starget->dev));
>  
>  	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
>  				&sdebug_target_reset_fail_fops);
> -	if (IS_ERR_OR_NULL(dentry))
> -		pr_info("%s: failed to create fail_reset file for target %s\n",
> -			__func__, dev_name(&starget->dev));
>  
>  	starget->hostdata = targetip;
>  


Thank you for the fix, the check for debugfs_create_file() is added because 
scsi_debug driver is often used to test abnormal situations, here just check
and prompt a log, so maybe you should not remove it and fix the issue
following changes:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 67922e2c4c19..d37437fa9eba 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1144,8 +1144,8 @@ static int sdebug_target_alloc(struct scsi_target *starget)
                pr_info("%s: failed to create debugfs directory for target %s\n",
                        __func__, dev_name(&starget->dev));
 
-       debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
-                               &sdebug_target_reset_fail_fops);
+       dentry = debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry,
+                                    starget, &sdebug_target_reset_fail_fops);
        if (IS_ERR_OR_NULL(dentry))
                pr_info("%s: failed to create fail_reset file for target %s\n",
                        __func__, dev_name(&starget->dev));
Dan Carpenter Oct. 23, 2023, 5:06 a.m. UTC | #2
On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
> On 2023/10/20 22:15, Dan Carpenter wrote:
> > Smatch complains that "dentry" is never initialized.  These days everyone
> > initializes all their stack variables to zero so this means that it will
> > trigger a warning every time this function is run.
> > 
> > Really debugfs functions are not supposed to be checked for errors so
> > this checking can just be deleted.
> > 
> > Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > See my blog for more information on the history of debugfs error
> > checking:
> > 
> > https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> > ---
> >  drivers/scsi/scsi_debug.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index 0a4e41d84df8..c0be9a53ac79 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -1127,7 +1127,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
> >  static int sdebug_target_alloc(struct scsi_target *starget)
> >  {
> >  	struct sdebug_target_info *targetip;
> > -	struct dentry *dentry;
> >  
> >  	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
> >  	if (!targetip)
> > @@ -1135,15 +1134,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
> >  
> >  	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
> >  				sdebug_debugfs_root);
> > -	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
> > -		pr_info("%s: failed to create debugfs directory for target %s\n",
> > -			__func__, dev_name(&starget->dev));
> >  
> >  	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
> >  				&sdebug_target_reset_fail_fops);
> > -	if (IS_ERR_OR_NULL(dentry))
> > -		pr_info("%s: failed to create fail_reset file for target %s\n",
> > -			__func__, dev_name(&starget->dev));
> >  
> >  	starget->hostdata = targetip;
> >  
> 
> 
> Thank you for the fix, the check for debugfs_create_file() is added because 
> scsi_debug driver is often used to test abnormal situations, here just check
> and prompt a log, so maybe you should not remove it and fix the issue
> following changes:
> 

No, the correct thing is to remove it.  This is explained in my blog
article linked to earlier.

https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/

commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Date:   Wed Jan 23 11:28:14 2019 +0100

    debugfs: return error values, not NULL
    
    When an error happens, debugfs should return an error pointer value, not
    NULL.  This will prevent the totally theoretical error where a debugfs
    call fails due to lack of memory, returning NULL, and that dentry value
    is then passed to another debugfs call, which would end up succeeding,
    creating a file at the root of the debugfs tree, but would then be
    impossible to remove (because you can not remove the directory NULL).
    
    So, to make everyone happy, always return errors, this makes the users
    of debugfs much simpler (they do not have to ever check the return
    value), and everyone can rest easy.

In your code, if there is an error the debugfs code will print an error and
your code will print an info.  The info adds nothing.  Also if debugfs fails
to load you are already screwed so the info adds nothing.

In your code if the user disables CONFIG_DEBUGFS then printing "failed to create
fail_reset file for target" is wrong.  The user did that deliberately.  No need
to complain about the user's deliberate choices.  If it's really necessary to
have CONFIG_DEBUGFS then enforce that with Kconfig.

regards,
dan carpenter
Wenchao Hao Oct. 24, 2023, 4:49 p.m. UTC | #3
On 10/23/23 1:06 PM, Dan Carpenter wrote:
> On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
>> On 2023/10/20 22:15, Dan Carpenter wrote:
>>> Smatch complains that "dentry" is never initialized.  These days everyone
>>> initializes all their stack variables to zero so this means that it will
>>> trigger a warning every time this function is run.
>>>
>>> Really debugfs functions are not supposed to be checked for errors so
>>> this checking can just be deleted.
>>>
>>> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>>> ---
>>> See my blog for more information on the history of debugfs error
>>> checking:
>>>
>>> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
>>> ---
>>>  drivers/scsi/scsi_debug.c | 7 -------
>>>  1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
>>> index 0a4e41d84df8..c0be9a53ac79 100644
>>> --- a/drivers/scsi/scsi_debug.c
>>> +++ b/drivers/scsi/scsi_debug.c
>>> @@ -1127,7 +1127,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
>>>  static int sdebug_target_alloc(struct scsi_target *starget)
>>>  {
>>>  	struct sdebug_target_info *targetip;
>>> -	struct dentry *dentry;
>>>  
>>>  	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
>>>  	if (!targetip)
>>> @@ -1135,15 +1134,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
>>>  
>>>  	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
>>>  				sdebug_debugfs_root);
>>> -	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
>>> -		pr_info("%s: failed to create debugfs directory for target %s\n",
>>> -			__func__, dev_name(&starget->dev));
>>>  
>>>  	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
>>>  				&sdebug_target_reset_fail_fops);
>>> -	if (IS_ERR_OR_NULL(dentry))
>>> -		pr_info("%s: failed to create fail_reset file for target %s\n",
>>> -			__func__, dev_name(&starget->dev));
>>>  
>>>  	starget->hostdata = targetip;
>>>  
>>
>>
>> Thank you for the fix, the check for debugfs_create_file() is added because 
>> scsi_debug driver is often used to test abnormal situations, here just check
>> and prompt a log, so maybe you should not remove it and fix the issue
>> following changes:
>>
> 
> No, the correct thing is to remove it.  This is explained in my blog
> article linked to earlier.
> 
> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> 

There are other places in scsi_debug which check return value
of debugfs functions added by my previous patches, would you
remove them?

Thanks


> commit ff9fb72bc07705c00795ca48631f7fffe24d2c6b
> Author: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Date:   Wed Jan 23 11:28:14 2019 +0100
> 
>     debugfs: return error values, not NULL
>     
>     When an error happens, debugfs should return an error pointer value, not
>     NULL.  This will prevent the totally theoretical error where a debugfs
>     call fails due to lack of memory, returning NULL, and that dentry value
>     is then passed to another debugfs call, which would end up succeeding,
>     creating a file at the root of the debugfs tree, but would then be
>     impossible to remove (because you can not remove the directory NULL).
>     
>     So, to make everyone happy, always return errors, this makes the users
>     of debugfs much simpler (they do not have to ever check the return
>     value), and everyone can rest easy.
> 
> In your code, if there is an error the debugfs code will print an error and
> your code will print an info.  The info adds nothing.  Also if debugfs fails
> to load you are already screwed so the info adds nothing.
> 
> In your code if the user disables CONFIG_DEBUGFS then printing "failed to create
> fail_reset file for target" is wrong.  The user did that deliberately.  No need
> to complain about the user's deliberate choices.  If it's really necessary to
> have CONFIG_DEBUGFS then enforce that with Kconfig.
> 
> regards,
> dan carpenter
Dan Carpenter Oct. 25, 2023, 4:07 a.m. UTC | #4
On Wed, Oct 25, 2023 at 12:49:48AM +0800, Wenchao Hao wrote:
> On 10/23/23 1:06 PM, Dan Carpenter wrote:
> > On Sat, Oct 21, 2023 at 01:28:50AM +0800, Wenchao Hao wrote:
> >> On 2023/10/20 22:15, Dan Carpenter wrote:
> >>> Smatch complains that "dentry" is never initialized.  These days everyone
> >>> initializes all their stack variables to zero so this means that it will
> >>> trigger a warning every time this function is run.
> >>>
> >>> Really debugfs functions are not supposed to be checked for errors so
> >>> this checking can just be deleted.
> >>>
> >>> Fixes: f084fe52c640 ("scsi: scsi_debug: Add debugfs interface to fail target reset")
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >>> ---
> >>> See my blog for more information on the history of debugfs error
> >>> checking:
> >>>
> >>> https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> >>> ---
> >>>  drivers/scsi/scsi_debug.c | 7 -------
> >>>  1 file changed, 7 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> >>> index 0a4e41d84df8..c0be9a53ac79 100644
> >>> --- a/drivers/scsi/scsi_debug.c
> >>> +++ b/drivers/scsi/scsi_debug.c
> >>> @@ -1127,7 +1127,6 @@ static const struct file_operations sdebug_target_reset_fail_fops = {
> >>>  static int sdebug_target_alloc(struct scsi_target *starget)
> >>>  {
> >>>  	struct sdebug_target_info *targetip;
> >>> -	struct dentry *dentry;
> >>>  
> >>>  	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
> >>>  	if (!targetip)
> >>> @@ -1135,15 +1134,9 @@ static int sdebug_target_alloc(struct scsi_target *starget)
> >>>  
> >>>  	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
> >>>  				sdebug_debugfs_root);
> >>> -	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
> >>> -		pr_info("%s: failed to create debugfs directory for target %s\n",
> >>> -			__func__, dev_name(&starget->dev));
> >>>  
> >>>  	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
> >>>  				&sdebug_target_reset_fail_fops);
> >>> -	if (IS_ERR_OR_NULL(dentry))
> >>> -		pr_info("%s: failed to create fail_reset file for target %s\n",
> >>> -			__func__, dev_name(&starget->dev));
> >>>  
> >>>  	starget->hostdata = targetip;
> >>>  
> >>
> >>
> >> Thank you for the fix, the check for debugfs_create_file() is added because 
> >> scsi_debug driver is often used to test abnormal situations, here just check
> >> and prompt a log, so maybe you should not remove it and fix the issue
> >> following changes:
> >>
> > 
> > No, the correct thing is to remove it.  This is explained in my blog
> > article linked to earlier.
> > 
> > https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
> > 
> 
> There are other places in scsi_debug which check return value
> of debugfs functions added by my previous patches, would you
> remove them?

It's harmless to check most of the time.  Technically, they should be
removed but that's like a whitespace clean up so it's not worth spending
a lot of time on unless you care about a specific driver.  Here, though,
it was an uninitialized variable bug, and try to fix bugs.

Smatch is really about fixing bugs so I have special code to not warn
about if people check debugfs code for NULL.  Normally Smatch would
complain if people check an error pointer function for NULL but with
debugfs functions it doesn't matter.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 0a4e41d84df8..c0be9a53ac79 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -1127,7 +1127,6 @@  static const struct file_operations sdebug_target_reset_fail_fops = {
 static int sdebug_target_alloc(struct scsi_target *starget)
 {
 	struct sdebug_target_info *targetip;
-	struct dentry *dentry;
 
 	targetip = kzalloc(sizeof(struct sdebug_target_info), GFP_KERNEL);
 	if (!targetip)
@@ -1135,15 +1134,9 @@  static int sdebug_target_alloc(struct scsi_target *starget)
 
 	targetip->debugfs_entry = debugfs_create_dir(dev_name(&starget->dev),
 				sdebug_debugfs_root);
-	if (IS_ERR_OR_NULL(targetip->debugfs_entry))
-		pr_info("%s: failed to create debugfs directory for target %s\n",
-			__func__, dev_name(&starget->dev));
 
 	debugfs_create_file("fail_reset", 0600, targetip->debugfs_entry, starget,
 				&sdebug_target_reset_fail_fops);
-	if (IS_ERR_OR_NULL(dentry))
-		pr_info("%s: failed to create fail_reset file for target %s\n",
-			__func__, dev_name(&starget->dev));
 
 	starget->hostdata = targetip;