On Fri, Jul 01, 2011 at 05:43:39AM -0400, Christoph Hellwig wrote:
> In most places we can simply pass around and use the struct xfs_dir2_data_hdr,
> which is the first and most important member of struct xfs_dir2_data instead
> of the full structure.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Only a couple of minor things.
....
> @@ -251,12 +258,13 @@ xfs_dir2_data_freeinsert(
> xfs_dir2_data_free_t new; /* new bestfree entry */
>
> #ifdef __KERNEL__
> - ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> - be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> + ASSERT(be32_to_cpu(hdr->magic) == XFS_DIR2_DATA_MAGIC ||
> + be32_to_cpu(hdr->magic) == XFS_DIR2_BLOCK_MAGIC);
> #endif
You kill the ifdef __KERNEL__ there.
> @@ -286,36 +294,36 @@ xfs_dir2_data_freeinsert(
> */
> STATIC void
> xfs_dir2_data_freeremove(
> - xfs_dir2_data_t *d, /* data block pointer */
> + xfs_dir2_data_hdr_t *hdr, /* data block header */
> xfs_dir2_data_free_t *dfp, /* bestfree entry pointer */
> int *loghead) /* out: log data header */
> {
> #ifdef __KERNEL__
> - ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> - be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> + ASSERT(be32_to_cpu(hdr->magic) == XFS_DIR2_DATA_MAGIC ||
> + be32_to_cpu(hdr->magic) == XFS_DIR2_BLOCK_MAGIC);
> #endif
And there.
> @@ -335,23 +344,23 @@ xfs_dir2_data_freescan(
> char *p; /* current entry pointer */
>
> #ifdef __KERNEL__
> - ASSERT(be32_to_cpu(d->hdr.magic) == XFS_DIR2_DATA_MAGIC ||
> - be32_to_cpu(d->hdr.magic) == XFS_DIR2_BLOCK_MAGIC);
> + ASSERT(hdr->magic == cpu_to_be32(XFS_DIR2_DATA_MAGIC) ||
> + hdr->magic == cpu_to_be32(XFS_DIR2_BLOCK_MAGIC));
> #endif
I'll stop commenting on this now ;)
However, I have noticed that you've converted some of the magic
number compares to cpu_to_be32(XFS_DIR2_DATA_MAGIC) form and not
others. I'm not so concerned about the ASSERT()s, but some of the
real runtime checks are touched but not then not changed around.
Anyway, this can probably be done later as a separate cleanup.
> if (!needscan) {
> - xfs_dir2_data_freeremove(d, dfp, needlogp);
> - (void)xfs_dir2_data_freeinsert(d, newdup,
> + xfs_dir2_data_freeremove(hdr, dfp, needlogp);
> + (void)xfs_dir2_data_freeinsert(hdr, newdup,
> needlogp);
> - (void)xfs_dir2_data_freeinsert(d, newdup2,
> + (void)xfs_dir2_data_freeinsert(hdr, newdup2,
> needlogp);
> }
> }
Kill the (void) casts?
Otherwise looks good.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
