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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxx>
> 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
