On Tue, Sep 12, 2023 at 10:41:53AM -0500, Alex Elder wrote: > On 9/12/23 10:38 AM, Darrick J. Wong wrote: > > On Tue, Sep 12, 2023 at 06:18:45PM +0300, Dan Carpenter wrote: > > > The shmem_file_setup() function can't return NULL so there is no need > > > to check and doing so is a bit confusing. > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > > --- > > > No fixes tag because this is not a bug, just some confusing code. > > > > Please don't re-send patches that have already been presented here. > > https://lore.kernel.org/linux-xfs/20230824161428.GO11263@frogsfrogsfrogs/ > > FWIW, I side with Dan's argument. shmem_file_setup() *does not* > return NULL. If it ever *did* return NULL, it would be up to the > person who makes that happen to change all callers to check for NULL. And as I asked three weeks ago, what's the harm in checking for a NULL pointer here? The kerneldoc for shmem_file_setup doesn't explicitly exclude a null return. True, it doesn't mention the possibility of ERR_PTR returns either, but that's an accepted practice for pointer returns. For a call outside of the xfs subsystem, I think it's prudent to have stronger return value checking. Yes, someone changing the interface would have to add a null check to all the callsites, but (a) it's benign to guard against a behavior change in another module and (b) people miss things all the time. > The current code *suggests* that it could return NULL, which > is not correct. Huh? Are you talking about this stupid behavior of bots where they decide what a function returns based on the callsites in lieu of analyzing the actual implementation code? I don't feel like getting harassed by bots when someone /does/ accidentally change the implementation to return NULL, and now one of the other build/test/syz bots starts crashing in xfile_create. Of course all that has bought me is ... more f*** bot harassment. I'm BURNED OUT, give me a fucking break! --D > -Alex > > > > > --D > > > > > fs/xfs/scrub/xfile.c | 2 -- > > > 1 file changed, 2 deletions(-) > > > > > > diff --git a/fs/xfs/scrub/xfile.c b/fs/xfs/scrub/xfile.c > > > index d98e8e77c684..71779d81cad7 100644 > > > --- a/fs/xfs/scrub/xfile.c > > > +++ b/fs/xfs/scrub/xfile.c > > > @@ -70,8 +70,6 @@ xfile_create( > > > return -ENOMEM; > > > xf->file = shmem_file_setup(description, isize, 0); > > > - if (!xf->file) > > > - goto out_xfile; > > > if (IS_ERR(xf->file)) { > > > error = PTR_ERR(xf->file); > > > goto out_xfile; > > > -- > > > 2.39.2 > > > >
