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@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?
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
