| Message ID | 20210810052451.41578-3-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework feature flags | expand |
On Tue, Aug 10, 2021 at 03:24:37PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > xfs_has_attr() is poorly named. It has global scope as it is defined > in a header file, but it has no namespace scope that tells us what > it is checking has attributes. It's not even clear what "has_attr" > means, because what it is actually doing is an attribute fork lookup > to see if the attribute exists. > > Upcoming patches use this "xfs_has_<foo>" namespace for global > filesystem features, which conflicts with this function. > > Rename xfs_has_attr() to xfs_attr_lookup() and make it a static > function, freeing up the "xfs_has_" namespace for global scope > usage. Why not kill this function entirely as I suggested last time? --- From 154d64990a9f410200a117729149dc1febb02458 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Thu, 12 Aug 2021 10:01:08 +0200 Subject: xfs: remove xfs_has_attr xfs_has_attr() is poorly named. It has global scope as it is defined in a header file, but it has no namespace scope that tells us what it is checking has attributes. It's not even clear what "has_attr" means, because what it is actually doing is an attribute fork lookup to see if the attribute exists. Fold it into the only caller. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/libxfs/xfs_attr.c | 46 ++++++++++++---------------------------- fs/xfs/libxfs/xfs_attr.h | 1 - 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 010d499b237c55..939afdcec634be 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -638,36 +638,6 @@ xfs_attr_set_iter( return error; } - -/* - * Return EEXIST if attr is found, or ENOATTR if not - */ -int -xfs_has_attr( - struct xfs_da_args *args) -{ - struct xfs_inode *dp = args->dp; - struct xfs_buf *bp = NULL; - int error; - - if (!xfs_inode_hasattr(dp)) - return -ENOATTR; - - if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) - return xfs_attr_sf_findname(args, NULL, NULL); - - if (xfs_attr_is_leaf(dp)) { - error = xfs_attr_leaf_hasname(args, &bp); - - if (bp) - xfs_trans_brelse(args->trans, bp); - - return error; - } - - return xfs_attr_node_hasname(args, NULL); -} - /* * Remove the attribute specified in @args. */ @@ -780,8 +750,21 @@ xfs_attr_set( goto out_trans_cancel; } + if (!xfs_inode_hasattr(dp)) { + error = -ENOATTR; + } else if (dp->i_afp->if_format == XFS_DINODE_FMT_LOCAL) { + error = xfs_attr_sf_findname(args, NULL, NULL); + } else if (xfs_attr_is_leaf(dp)) { + struct xfs_buf *bp = NULL; + + error = xfs_attr_leaf_hasname(args, &bp); + if (bp) + xfs_trans_brelse(args->trans, bp); + } else { + error = xfs_attr_node_hasname(args, NULL); + } + if (args->value) { - error = xfs_has_attr(args); if (error == -EEXIST && (args->attr_flags & XATTR_CREATE)) goto out_trans_cancel; if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE)) @@ -796,7 +779,6 @@ xfs_attr_set( if (!args->trans) goto out_unlock; } else { - error = xfs_has_attr(args); if (error != -EEXIST) goto out_trans_cancel; diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 8de5d1d2733ead..5e71f719bdd52b 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -490,7 +490,6 @@ int xfs_attr_get_ilocked(struct xfs_da_args *args); int xfs_attr_get(struct xfs_da_args *args); int xfs_attr_set(struct xfs_da_args *args); int xfs_attr_set_args(struct xfs_da_args *args); -int xfs_has_attr(struct xfs_da_args *args); int xfs_attr_remove_args(struct xfs_da_args *args); int xfs_attr_remove_iter(struct xfs_delattr_context *dac); bool xfs_attr_namecheck(const void *name, size_t length);
On Thu, Aug 12, 2021 at 09:03:34AM +0100, Christoph Hellwig wrote: > On Tue, Aug 10, 2021 at 03:24:37PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > xfs_has_attr() is poorly named. It has global scope as it is defined > > in a header file, but it has no namespace scope that tells us what > > it is checking has attributes. It's not even clear what "has_attr" > > means, because what it is actually doing is an attribute fork lookup > > to see if the attribute exists. > > > > Upcoming patches use this "xfs_has_<foo>" namespace for global > > filesystem features, which conflicts with this function. > > > > Rename xfs_has_attr() to xfs_attr_lookup() and make it a static > > function, freeing up the "xfs_has_" namespace for global scope > > usage. > > Why not kill this function entirely as I suggested last time? Because I think it's the wrong cleanup to apply to xfs_attr_set(). xfs_attr_set() needs to be split into two - a set() function and a remove() function to get rid of all the conditional "if (arg->value)" logic in it that separates set from remove. Most of the code in the function is under such if/else clauses, and the set() code is much more complex than the remove() case. Folding the attr lookup into the xfs_attr_set() doesn't do anything to address this high level badness, and to split it appropriately we need to keep the common attr lookup code in it's own function. I updated the patch to has a single xfs_attr_lookup() call instead of one per branch, but I don't think removing the helper is the right way to go here... Cheers, Dave.
On Wed, Aug 18, 2021 at 10:56:08AM +1000, Dave Chinner wrote: > On Thu, Aug 12, 2021 at 09:03:34AM +0100, Christoph Hellwig wrote: > > On Tue, Aug 10, 2021 at 03:24:37PM +1000, Dave Chinner wrote: > > > From: Dave Chinner <dchinner@redhat.com> > > > > > > xfs_has_attr() is poorly named. It has global scope as it is defined > > > in a header file, but it has no namespace scope that tells us what > > > it is checking has attributes. It's not even clear what "has_attr" > > > means, because what it is actually doing is an attribute fork lookup > > > to see if the attribute exists. > > > > > > Upcoming patches use this "xfs_has_<foo>" namespace for global > > > filesystem features, which conflicts with this function. > > > > > > Rename xfs_has_attr() to xfs_attr_lookup() and make it a static > > > function, freeing up the "xfs_has_" namespace for global scope > > > usage. > > > > Why not kill this function entirely as I suggested last time? > > Because I think it's the wrong cleanup to apply to xfs_attr_set(). > xfs_attr_set() needs to be split into two - a set() function and a > remove() function to get rid of all the conditional "if > (arg->value)" logic in it that separates set from remove. Most > of the code in the function is under such if/else clauses, and the > set() code is much more complex than the remove() case. Folding the > attr lookup into the xfs_attr_set() doesn't do anything to address > this high level badness, and to split it appropriately we need to > keep the common attr lookup code in it's own function. > > I updated the patch to has a single xfs_attr_lookup() call instead > of one per branch, but I don't think removing the helper is the > right way to go here... I prefer we leave the xattr code alone (renaming things to avoid name conflicts is ok) and concentrated on helping Allison land xattr logging. Once that's done, xfs_attr_set and xfs_attr_remove become trivial frontends to a deferred operation. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c index 010d499b237c..6771bd700770 100644 --- a/fs/xfs/libxfs/xfs_attr.c +++ b/fs/xfs/libxfs/xfs_attr.c @@ -642,8 +642,8 @@ xfs_attr_set_iter( /* * Return EEXIST if attr is found, or ENOATTR if not */ -int -xfs_has_attr( +static int +xfs_attr_lookup( struct xfs_da_args *args) { struct xfs_inode *dp = args->dp; @@ -781,7 +781,7 @@ xfs_attr_set( } if (args->value) { - error = xfs_has_attr(args); + error = xfs_attr_lookup(args); if (error == -EEXIST && (args->attr_flags & XATTR_CREATE)) goto out_trans_cancel; if (error == -ENOATTR && (args->attr_flags & XATTR_REPLACE)) @@ -796,7 +796,7 @@ xfs_attr_set( if (!args->trans) goto out_unlock; } else { - error = xfs_has_attr(args); + error = xfs_attr_lookup(args); if (error != -EEXIST) goto out_trans_cancel; diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h index 8de5d1d2733e..5e71f719bdd5 100644 --- a/fs/xfs/libxfs/xfs_attr.h +++ b/fs/xfs/libxfs/xfs_attr.h @@ -490,7 +490,6 @@ int xfs_attr_get_ilocked(struct xfs_da_args *args); int xfs_attr_get(struct xfs_da_args *args); int xfs_attr_set(struct xfs_da_args *args); int xfs_attr_set_args(struct xfs_da_args *args); -int xfs_has_attr(struct xfs_da_args *args); int xfs_attr_remove_args(struct xfs_da_args *args); int xfs_attr_remove_iter(struct xfs_delattr_context *dac); bool xfs_attr_namecheck(const void *name, size_t length);