Re: [PATCH 1/3] btrfs-progs: check/lowmem: Add check and repair for invalid inode generation

[Qu Wenruo]

On 2019/9/30 下午9:34, Nikolay Borisov wrote:
>
>
> On 30.09.19 г. 15:24 ч., Qu Wenruo wrote:
>> Yes, the ASSERT() doesn't make much sense by itself.
>>
>> However I still believe it won't be a problem.
>
> It won't be a problem but it feels wrong to have this assert this deep
> into the call chain. IMO It should be put where it can trigger at the
> earliest which seems to be in check_inode_item. That function assumes
> it's working with an inode item and goes to dereference inode members so
> if the type is wrong we'd crash there instead of in repair_inode_gen_lowmem.

This is going to be a preference thing.

To my taste, if possible, I would put ASSERT() in every function with
more than 24 lines, as an alternative to comment, to show the
prerequisite or the assumption.

Thanks,
Qu

>
>>
>> It's compiler's job to remove such dead ASSERT(), but for human reader,
>> I still believe this ASSERT() could still make sense, especially when
>> the caller or callee can get more and more complex.
>>
>> Thanks,