Re: [PATCH V2 02/10] xfs: Check for extent overflow when trivally adding a new extent

[Chandan Babu R <chandanrlinux@gmail.com>]

On Thursday 27 August 2020 1:39:03 PM IST Christoph Hellwig wrote:
> On Mon, Aug 17, 2020 at 01:14:16PM +0530, Chandan Babu R wrote:
> > > > +		error = xfs_iext_count_may_overflow(ip, whichfork,
> > > > +				XFS_IEXT_ADD_CNT);
> > > 
> > > I find the XFS_IEXT_ADD_CNT define very confusing.  An explicit 1 passed
> > > for a counter parameter makes a lot more sense to me.
> > 
> > The reason to do this was to consolidate the comment descriptions at one
> > place. For e.g. the comment for XFS_IEXT_DIR_MANIP_CNT (from "[PATCH V2 05/10]
> > xfs: Check for extent overflow when adding/removing dir entries") is slightly
> > larger. Using constants (instead of macros) would mean that the same comment
> > has to be replicated across the 6 locations it is being used.
> 
> I agree with a constant if we have a complex computed value.  But a
> constant for 1 where it is obvious from the context that one means
> the number one as in adding a single items is just silly and really
> hurts when reading the code.

I think we should retain the macros because there are nine macros out of which
three macros do trivial amounts of computation rather than having literal
integers as values. Having macros for these three while using literal integers
for other six cases would IMHO make the code non-uniform. If we end up
removing the macros completely we would have two problems,
1. Redundant code comments sprinkled across the code base explaining the logic
   behind computing the "extent delta".
2. In the future, if the "extent delta" value has to be changed for an
   operation, it has to be changed across all the relevant invocations of
   xfs_iext_count_may_overflow().

-- 
chandan