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