Re: [PATCH 01/14] repair: merge scanfunc_bno and scanfunc_cnt

[Eric Sandeen <sandeen@sandeen.net>]

Christoph Hellwig wrote:

> Those two functions are almost identical. The big difference is that 
> we only
> move blocks from XR_E_FREE1 to XR_E_FREE state when processing the cnt btree.
>
> Besides that we print bno vs cnt in the messages and obviously validate a
> slightly different magic number in the header.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>

Generally seems fine to me, a couple of nitpicks below, take 'em or leave 'em.

> Index: xfsprogs-dev/repair/scan.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/scan.c	2009-08-21 18:24:26.000000000 +0000
> +++ xfsprogs-dev/repair/scan.c	2009-08-21 18:40:59.000000000 +0000
> @@ -439,15 +439,16 @@ _("out-of-order bmap key (file offset) i
>  }
>  
>  void
> -scanfunc_bno(
> +scanfunc_allocbt(
>  	struct xfs_btree_block	*block,
>  	int			level,
>  	xfs_agblock_t		bno,
>  	xfs_agnumber_t		agno,
>  	int			suspect,
> -	int			isroot
> -	)
> +	int			isroot,
> +	__uint32_t		magic)
>  {
> +	const char 		*name;
>  	xfs_agblock_t		b, e;
>  	int			i;
>  	xfs_alloc_ptr_t		*pp;
> @@ -456,16 +457,18 @@ scanfunc_bno(
>  	int			numrecs;
>  	int			state;
>  
> -	if (be32_to_cpu(block->bb_magic) != XFS_ABTB_MAGIC) {
> -		do_warn(_("bad magic # %#x in btbno block %d/%d\n"),
> -			be32_to_cpu(block->bb_magic), agno, bno);
> +	name = (magic == XFS_ABTB_MAGIC) ? "bno" : "cnt";

Should we explicitly test that this is either 
XFS_ABTC_MAGIC or XFS_ABTB_MAGIC here to avoid any programming-error
type problems?
> +
> +	if (be32_to_cpu(block->bb_magic) != magic) {
> +		do_warn(_("bad magic # %#x in bt%s block %d/%d\n"),
> +			be32_to_cpu(block->bb_magic), name, agno, bno);
>  		hdr_errors++;
>  		if (suspect)
>  			return;
>  	}
>  	if (be16_to_cpu(block->bb_level) != level) {
> -		do_warn(_("expected level %d got %d in btbno block %d/%d\n"),
> -			level, be16_to_cpu(block->bb_level), agno, bno);
> +		do_warn(_("expected level %d got %d in bt%s block %d/%d\n"),
> +			level, be16_to_cpu(block->bb_level), name, agno, bno);
>  		hdr_errors++;
>  		if (suspect)
>  			return;
> @@ -483,8 +486,8 @@ scanfunc_bno(
>  	default:
>  		set_agbno_state(mp, agno, bno, XR_E_MULT);
>  		do_warn(
> -_("bno freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
> -				state, agno, bno, suspect);
> +_("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
> +				name, state, agno, bno, suspect);
>  		return;
>  	}
>  
> @@ -520,15 +523,27 @@ _("bno freespace btree block claimed (st
>  				continue;
>  			for (b = be32_to_cpu(rp[i].ar_startblock);
>  			     b < e; b++)  {
> -				if (get_agbno_state(mp, agno, b)
> -							== XR_E_UNKNOWN)
> +				state = get_agbno_state(mp, agno, b);
> +				switch (state) {
> +				case XR_E_UNKNOWN:
>  					set_agbno_state(mp, agno, b,
>  							XR_E_FREE1);
> -				else  {
> +					break;
> +				case XR_E_FREE1:
> +					/*
> +					 * no warning messages -- we'll catch
> +					 * FREE1 blocks later
> +					 */
> +					if (magic != XFS_ABTB_MAGIC) {

Why not make this explicitly "if (magic == XFS_ABTC_MAGIC)" - I guess it seems potentially
more future-proof to me though I don't suppose we'll ever get a new type here.  :)
The positive test seems clearer to me but *shrug*.

Rest looks fine.  I suppose we should do the same to the functions in db/* someday.

Thanks,
-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs