Re: [PATCH 5/8] xfs: remove xfs_attr_shortform_lookup

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

On Tue, Dec 19, 2023 at 03:46:27PM +0100, Christoph Hellwig wrote:
> So, the buildbot rightly complained that this can return an
> uninitialized error variable in xfs_attr_shortform_addname now
> (why are we disabling the goddam use of uninitialized variable
> warnings in Linux again, sigh..).
> 
> I then not only created the trivial fix, but also wrote a simple wrapper
> for the setxattr syscall as the existing setfattr and attr tools don't
> allow to control the flag, which I assumed means xfstests didn't really
> test this as much as it should.  But that little test showed we're still
> getting the right errno values even with the unininitialized variable
> returns, which seemed odd.
> 
> It turns out we're not even exercising this code any more, as
> xfs_attr_set already does a xfs_attr_lookup lookup first and has a
> copy of this logic executed much earlier (and I should have really though
> about that because I got very close to that code for the defer ops
> cleanup).

Eh, there's lots of, uh, cleanup opportunities in the xattr code. ;)

The changes below look reasonable, but I wonder -- the leaf and node add
functions do a similar thing; can they go too?

I'm assuming those can't go away because they actually set @args->index
and @args->rmt* and we might've blown that away after the initial lookup
in xfs_attr_set?  But maybe they can?  Insofar as figuring all that out
is probably an entire campaign on its own.

> So..  I'm tempted to just turn these checks into asserts with something
> like the below on top of this patch, I'll just need to see if it survives
> testing:

I'll await your return then. :)

--D

> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index d6173888ed0d56..abdc58f286154a 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1066,13 +1066,13 @@ xfs_attr_shortform_addname(
>  	struct xfs_da_args	*args)
>  {
>  	int			newsize, forkoff;
> -	int			error;
>  
>  	trace_xfs_attr_sf_addname(args);
>  
>  	if (xfs_attr_sf_findname(args)) {
> -		if (!(args->op_flags & XFS_DA_OP_REPLACE))
> -			return error;
> +		int		error;
> +
> +		ASSERT(args->op_flags & XFS_DA_OP_REPLACE);
>  
>  		error = xfs_attr_sf_removename(args);
>  		if (error)
> @@ -1086,8 +1086,7 @@ xfs_attr_shortform_addname(
>  		 */
>  		args->op_flags &= ~XFS_DA_OP_REPLACE;
>  	} else {
> -		if (args->op_flags & XFS_DA_OP_REPLACE)
> -			return error;
> +		ASSERT(!(args->op_flags & XFS_DA_OP_REPLACE));
>  	}
>  
>  	if (args->namelen >= XFS_ATTR_SF_ENTSIZE_MAX ||
> 
>