Re: [PATCH] xfs: delete some dead code in xfile_create()

["Darrick J. Wong" <djwong@kernel.org>]

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@linaro.org>
> > > ---
> > > 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
> > > 
>