Re: [PATCH 02/16] xfs: rename xfs_has_attr()

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

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