Re: [PATCH 3/7] xfs: pass a xfs_efi_log_item to xfs_efi_item_sizeof

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

On Wed, Apr 21, 2021 at 07:56:28AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 20, 2021 at 10:09:52AM -0700, Darrick J. Wong wrote:
> > On Mon, Apr 19, 2021 at 10:28:00AM +0200, Christoph Hellwig wrote:
> > > xfs_efi_log_item only looks at the embedded xfs_efi_log_item structure,
> > > so pass that directly and rename the function to xfs_efi_log_item_sizeof.
> > > This allows using the helper in xlog_recover_efi_commit_pass2 as well.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  fs/xfs/xfs_extfree_item.c | 15 +++++++--------
> > >  1 file changed, 7 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_extfree_item.c b/fs/xfs/xfs_extfree_item.c
> > > index ed8d0790908ea7..7ae570d1944590 100644
> > > --- a/fs/xfs/xfs_extfree_item.c
> > > +++ b/fs/xfs/xfs_extfree_item.c
> > > @@ -70,11 +70,11 @@ xfs_efi_release(
> > >   * structure.
> > >   */
> > >  static inline int
> > > -xfs_efi_item_sizeof(
> > > -	struct xfs_efi_log_item *efip)
> > > +xfs_efi_log_item_sizeof(
> > 
> > Shouldn't this be named xfs_efi_log_format_sizeof to correspond to the
> > name of the structure?  Two patches from now you (re)introduce
> > xfs_efi_item_sizeof that returns the size of a struct xfs_efi_log_item,
> > which is confusing.
> 
> Well, that was the main reason to move these out of place, as they are
> rather misnamed.

I agree with the motivation, but not the names you've picked.  I don't
like xfs_efi_log_item_sizeof /not/ being the sizing function for struct
xfs_efi_log_item, because (in my head anyway) XXX_sizeof should be the
function for struct XXX.

IOWs I think that in the end these two helpers should be:

static inline int
xfs_efi_log_format_sizeof(
	struct xfs_efi_log_format *elf)
{
	return sizeof(*elf) + elf->efi_nextents * sizeof(struct xfs_extent);
}

static inline int
xfs_efi_log_item_sizeof(unsigned int nextents)
{
	return sizeof(struct xfs_efi_log_item) + nextents * sizeof(struct xfs_extent);
}

--D