On 7/25/22 03:42, Mariusz Tkaczyk wrote:
> On Sat, 23 Jul 2022 14:37:11 +1000
> "NeilBrown" <neilb@xxxxxxx> wrote:
>
>> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:
>>> Hi Neil,
>>>
>>> On Wed, 13 Jul 2022 13:48:11 +1000
>>> "NeilBrown" <neilb@xxxxxxx> wrote:
....
>>> why not just:
>>> if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size) >
>>> dsize) from my understanding, only this check matters.
>>
>> It seemed safest to test both. I don't remember the difference between
>> ->size and ->data_size. In getinfo_super1() we have
>>
>> if (info->array.level <= 0)
>> data_size = __le64_to_cpu(sb->data_size);
>> else
>> data_size = __le64_to_cpu(sb->size);
>>
>> which suggests that either could be relevant.
>> I guess ->size should always be less than ->data_size. But
>> load_super1() doesn't check that, so it isn't safe to assume it.
>
> Honestly, I don't understand why but I didn't realize that you are checking two
> different fields (size and data_size). I focused on understanding what is going
> on here, and didn't catch difference in variables (because data_offset and
> data_size have similar prefix).
> For me, something like:
>
> unsigned long long _size;
> if (st->minor_version >= 1 && st->ignore_hw_compat == 0)
> _size= __le64_to_cpu(super->size);
> else
> _size= __le64_to_cpu(super->data_size);
>
> if (__le64_to_cpu(super->data_offset) + _size > dsize)
> {....}
>
> is more readable because I don't need to analyze complex if to get the
> difference. Also, I removed doubled __le64_to_cpu(super->data_offset).
> Could you refactor this part?
What is the consensus on this discussion? I see Coly pulled this into
his tree, but I don't see Mariusz's last concern being addressed.
Thanks,
Jes
