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

Eric Sandeen
2009-10-12T16:51:32Z

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