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@xxxxxx>
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
|