On Wed, Apr 26, 2017 at 01:41:26PM -0500, Eric Sandeen wrote:
> On 4/26/17 1:25 PM, Darrick J. Wong wrote:
> > On Wed, Apr 26, 2017 at 12:48:48PM -0500, Eric Sandeen wrote:
> >> On 4/10/17 7:20 PM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >>>
> >>> The function block_to_bt plays an integral role in determining the btree
> >>> geometry of a block that we want to manipulate with the debugger.
> >>> Normally we use the block magic to find the geometry profile, but if the
> >>> magic is bad we'll never find it and return NULL. The callers of this
> >>> function do not check for NULL and crash.
> >>>
> >>> Therefore, if we can't find a geometry profile matching the magic
> >>> number, use the iocursor type to guess the profile and scowl about that
> >>> to stdout. This makes it so that even with a corrupt magic we can try
> >>> to print the fields instead of crashing the debugger.
> >>>
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >>
> >> Makes sense generally, minor things below.
> >>
> >>> ---
> >>> v2: be less macro-happy and only evaluate hascrc once
> >>> v3: braces around the for loop body
> >>> ---
> >>> db/btblock.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++--------
> >>> 1 file changed, 46 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/db/btblock.c b/db/btblock.c
> >>> index 835a5f0..b819fa5 100644
> >>> --- a/db/btblock.c
> >>> +++ b/db/btblock.c
> >>> @@ -25,6 +25,8 @@
> >>> #include "print.h"
> >>> #include "bit.h"
> >>> #include "init.h"
> >>> +#include "io.h"
> >>> +#include "output.h"
> >>>
> >>> /*
> >>> * Definition of the possible btree block layouts.
> >>> @@ -122,13 +124,52 @@ static struct xfs_db_btree *
> >>
> >> urgh:
> >>
> >> /*
> >> * Find the right block defintion for a given ondisk block.
> >> *
> >> * We use the least significant bit of the magic number as index into
> >> * the array of block defintions.
> >>
> >> (nope, we don't, I can remove that on commit...)
> >
> > Oops. Should've nuked that comment from the start.
>
> Ok, thanks for the replies.
>
> I'll:
>
> 1) Fix up comments
> 2) /Not/ do backwards-jumping-gotos, fair point.
> 3) add the magic ASSERT
> 4) nuke the other ASSERTs
>
> on commit? or do you want to resend ;)
Fix it up, send it out one more time to the list?
--D
>
> >
> >> */
> >> static struct xfs_db_btree *
> >>
> >>> block_to_bt(
> >>> struct xfs_btree_block *bb)
> >>> {
> >>> - struct xfs_db_btree *btp = &btrees[0];
> >>> + struct xfs_db_btree *btp;
> >>> + uint32_t magic;
> >>> + bool crc;
> >>>
> >>> - do {
> >>> - if (be32_to_cpu((bb)->bb_magic) == btp->magic)
> >>> + magic = be32_to_cpu((bb)->bb_magic);
> >>
> >> maybe -
> >>
> >> again:
> >
> > Eww, backwards jumping gotos :)
> >
> > I guess that works so long as you're sure we can't accidentally jump
> > backwards with an unrecognized magic and thus end up in an infinite
> > loop.
> >
> >>> + for (btp = &btrees[0]; btp->magic != 0; btp++) {
> >>> + if (magic == btp->magic)
> >>> + return btp;
> >>> + }
> >>
> >> /* Magic is invalid/unknown. Guess based on iocur type */
> >>
> >>> + crc = xfs_sb_version_hascrc(&mp->m_sb);
> >>
> >> Bah, wish we could use something like:
> >>
> >> magic = xfs_magics[crc][btnum];
> >>
> >> but that's static to libxfs/xfs_btree.c and I guess btnum != typnm.
> >> Because of course.
> >>
> >> (btnum = xfs_typnm_to_btnum[iocur_top->typ->typnm]; ? :) Ok I'll
> >> drop it...)
> >
> > :D
> >
> >>> + switch (iocur_top->typ->typnm) {
> >>> + case TYP_BMAPBTA:
> >>> + case TYP_BMAPBTD:
> >>> + magic = crc ? XFS_BMAP_CRC_MAGIC : XFS_BMAP_MAGIC;
> >>> + break;
> >>> + case TYP_BNOBT:
> >>> + magic = crc ? XFS_ABTB_CRC_MAGIC : XFS_ABTB_MAGIC;
> >>> + break;
> >>> + case TYP_CNTBT:
> >>> + magic = crc ? XFS_ABTC_CRC_MAGIC : XFS_ABTC_MAGIC;
> >>> + break;
> >>> + case TYP_INOBT:
> >>> + magic = crc ? XFS_IBT_CRC_MAGIC : XFS_IBT_MAGIC;
> >>> + break;
> >>> + case TYP_FINOBT:
> >>> + magic = crc ? XFS_FIBT_CRC_MAGIC : XFS_FIBT_MAGIC;
> >>> + break;
> >>> + case TYP_RMAPBT:
> >>> + magic = crc ? XFS_RMAP_CRC_MAGIC : 0;
> >>> + break;
> >>> + case TYP_REFCBT:
> >>> + magic = crc ? XFS_REFC_CRC_MAGIC : 0;
> >>> + break;
> >>> + default:
> >>> + ASSERT(0);
> >>> + }
> >>
> >> ASSERT(magic);
> >>
> >> we'd better have that by now, yes?
> >
> > Yes.
> >
> >> Or could we hit the RMAP/REFC types w/o crc set?
> >
> > In theory, no, because xfs_db sets up a different type profile for the
> > !crc case, and that type profile excludes TYP_RMAPBT/TYP_REFCBT.
> >
> >> I guess then the caller ASSERTs anyway?
> >
> > <shrug>
> >
> >>> +
> >>> + dbprintf(_("Bad btree magic 0x%x; coercing to %s.\n"),
> >>> + be32_to_cpu((bb)->bb_magic),
> >>> + iocur_top->typ->name);
> >>
> >>
> >> goto again;
> >>
> >> to avoid the cut & paste of the loop below? But maybe that's uglier.
> >
> > Mostly a matter of taste, I think. If the machinery was more complex
> > I'd argue for a backwards goto to avoid having to maintain two identical
> > loops, but this is pretty simple.
> >
> >>> +
> >>> + for (btp = &btrees[0]; btp->magic != 0; btp++) {
> >>> + if (magic == btp->magic)
> >>> return btp;
> >>> - btp++;
> >>> - } while (btp->magic != 0);
> >>> + }
> >>>
> >>> return NULL;
> >>> }
> >>> @@ -193,7 +234,6 @@ btblock_key_offset(
> >>> int offset;
> >>>
> >>> ASSERT(startoff == 0);
> >>> - ASSERT(block->bb_level != 0);
> >>
> >> Help me out, why are these now removed?
> >
> > /me doesn't remember, maybe they should just go away.
> >
> >> -Eric
> >>
> >>>
> >>> offset = bt->block_len + (idx - 1) * bt->key_len;
> >>> return bitize(offset);
> >>> @@ -214,7 +254,6 @@ btblock_ptr_offset(
> >>> int maxrecs;
> >>>
> >>> ASSERT(startoff == 0);
> >>> - ASSERT(block->bb_level != 0);
> >>>
> >>> maxrecs = btblock_maxrecs(bt, mp->m_sb.sb_blocksize);
> >>> offset = bt->block_len +
> >>> @@ -238,7 +277,6 @@ btblock_rec_offset(
> >>> int offset;
> >>>
> >>> ASSERT(startoff == 0);
> >>> - ASSERT(block->bb_level == 0);
> >>>
> >>> offset = bt->block_len + (idx - 1) * bt->rec_len;
> >>> return bitize(offset);
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
