Re: [PATCH 17/27] xfs: kill struct xfs_dir2_block

Dave Chinner
2011-07-06T02:32:02Z

On Fri, Jul 01, 2011 at 05:43:38AM -0400, Christoph Hellwig wrote:
> Remove the confusing xfs_dir2_block structure. It is supposed to describe
> an XFS dir2 block format btree block, but due to the variable sized nature
> of almost all elements in it it can't actuall do anything close to that
> job. In addition to accessing the fixed offset header structure it was
> only used to get a pointer to the first dir or unused entry after it,
> which can be trivially replaced by pointer arithmetics on the header
> pointer. For most users that is actually more natural anyway, as they
> don't use a typed pointer but rather a character pointer for further
> arithmetics.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
.....
> @@ -471,14 +470,13 @@ xfs_dir2_block_getdents(
> * We'll skip entries before this.
> */
> wantoff = xfs_dir2_dataptr_to_off(mp, *offset);
> - block = bp->data;
> - hdr = &block->hdr;
> + hdr = bp->data;
> xfs_dir2_data_check(dp, bp);
> /*
> * Set up values for the loop.
> */
> btp = xfs_dir2_block_tail_p(mp, hdr);
> - ptr = (char *)block->u;
> + ptr = (char *)(hdr + 1);
> endptr = (char *)xfs_dir2_block_leaf_p(btp);

That is slightly less obvious what it is doing. It's jumping over
the entire header, but could easily be confused with jumping one
byte in.

Perhaps adding a wrapper e.g. xfs_dir2_block_data_p(hdr) to match
the xfs_dir2_block_tail_p() and xfs_dir2_block_leaf_p() wrappers,
and converting all the other cases to use this as well?

> @@ -1103,7 +1099,7 @@ xfs_dir2_sf_to_block(
> * The whole thing is initialized to free by the init routine.
> * Say we're using the leaf and tail area.
> */
> - dup = (xfs_dir2_data_unused_t *)block->u;
> + dup = (xfs_dir2_data_unused_t *)(hdr + 1);

and maybe a xfs_dir2_block_unused_p() wrapper just to avoid the cast
here, though I'm not sure it's worth adding a wrapper just for this
one use.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com