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 ;)
>
>> */
>> 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
