| Message ID | 20231219120817.923421-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] xfs: make if_data a void pointer | expand |
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).
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:
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 ||
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 || > >
On Tue, Dec 19, 2023 at 09:45:05AM -0800, Darrick J. Wong wrote: > 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. Yeah, this looks pretty scary to touch for a cleanup series that's already gone kinda out of bounds.. > > > 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. :) It has been surviving testing just fine over night.
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index bcf8748cb1a333..d6173888ed0d56 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -1070,13 +1070,7 @@ xfs_attr_shortform_addname( trace_xfs_attr_sf_addname(args); - error = xfs_attr_shortform_lookup(args); - switch (error) { - case -ENOATTR: - if (args->op_flags & XFS_DA_OP_REPLACE) - return error; - break; - case -EEXIST: + if (xfs_attr_sf_findname(args)) { if (!(args->op_flags & XFS_DA_OP_REPLACE)) return error; @@ -1091,11 +1085,9 @@ xfs_attr_shortform_addname( * around. */ args->op_flags &= ~XFS_DA_OP_REPLACE; - break; - case 0: - break; - default: - return error; + } else { + if (args->op_flags & XFS_DA_OP_REPLACE) + return error; } if (args->namelen >= XFS_ATTR_SF_ENTSIZE_MAX || diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c index 7a623efd23a6a4..75c597805ffa8b 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.c +++ b/fs/xfs/libxfs/xfs_attr_leaf.c @@ -837,30 +837,6 @@ xfs_attr_sf_removename( return 0; } -/* - * Look up a name in a shortform attribute list structure. - */ -/*ARGSUSED*/ -int -xfs_attr_shortform_lookup( - struct xfs_da_args *args) -{ - struct xfs_ifork *ifp = &args->dp->i_af; - struct xfs_attr_shortform *sf = ifp->if_data; - struct xfs_attr_sf_entry *sfe; - int i; - - ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL); - sfe = &sf->list[0]; - for (i = 0; i < sf->hdr.count; - sfe = xfs_attr_sf_nextentry(sfe), i++) { - if (xfs_attr_match(args, sfe->namelen, sfe->nameval, - sfe->flags)) - return -EEXIST; - } - return -ENOATTR; -} - /* * Retrieve the attribute value and length. * diff --git a/fs/xfs/libxfs/xfs_attr_leaf.h b/fs/xfs/libxfs/xfs_attr_leaf.h index 56fcd689eedfe7..35e668ae744fb1 100644 --- a/fs/xfs/libxfs/xfs_attr_leaf.h +++ b/fs/xfs/libxfs/xfs_attr_leaf.h @@ -47,7 +47,6 @@ struct xfs_attr3_icleaf_hdr { */ void xfs_attr_shortform_create(struct xfs_da_args *args); void xfs_attr_shortform_add(struct xfs_da_args *args, int forkoff); -int xfs_attr_shortform_lookup(struct xfs_da_args *args); int xfs_attr_shortform_getvalue(struct xfs_da_args *args); int xfs_attr_shortform_to_leaf(struct xfs_da_args *args); int xfs_attr_sf_removename(struct xfs_da_args *args);