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

[Dave Chinner <david@fromorbit.com>]

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.
-- 
Dave Chinner
david@fromorbit.com