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

[Wenchao Hao <haowenchao22@gmail.com>]

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