Re: [PATCH v5 08/14] xfs: Factor up xfs_attr_try_sf_addname

Allison Collins
2019-12-27T03:23:38Z

On 12/24/19 5:25 AM, Christoph Hellwig wrote:
> On Wed, Dec 11, 2019 at 09:15:07PM -0700, Allison Collins wrote:
>> New delayed attribute routines cannot handle transactions. We
>> can factor up the commit, but there is little left in this
>> function other than some error handling and an ichgtime. So
>> hoist all of xfs_attr_try_sf_addname up at this time. We will
>> remove all the commits in this set.
>
> I really don't like this one. xfs_attr_try_sf_addname is a nice
> abstration, so merging it into the caller makes the code much harder
> to understand. If that is different after changes to the transaction
> change it can be removed at that point, but merging all the different
> attr formats into one big monster function is a bad idea.
Well, in the last version of the set, I did keep it around, but it was
so small, Darrick suggested cleaning it out all together. Here is the
same patch in the last set:

https://patchwork.kernel.org/patch/11231511/

Maybe when more folks get back from break they can chime in about
keeping it or not. I think I commented pretty well on "monster
function" in the last patch. It is in all in the pursuit of pulling
things up and then breaking them back down again.

>
> Also Factor up still sounds very, very strange to me. I would
> have worded it as "merge xfs_attr_try_sf_addname into its only caller"
>
I always assumed it to be a figure of speech in reference to factoring
out common code, sort of like how one might factor out a common
denominator or common multiple from an algebraic expression. If it
really bothers folks though, I don't mind changing it.

Allison