On Thu, 25 Aug 2022 10:24:21 +1000
"NeilBrown" [off-list ref] wrote:

On Thu, 25 Aug 2022, Jes Sorensen wrote:

quoted

On 7/25/22 03:42, Mariusz Tkaczyk wrote:  

quoted

On Sat, 23 Jul 2022 14:37:11 +1000
"NeilBrown" [off-list ref] wrote:
  

quoted

On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:  

quoted

Hi Neil,

On Wed, 13 Jul 2022 13:48:11 +1000
"NeilBrown" [off-list ref] wrote:  

....  

quoted

quoted

quoted

why not just:
if (__le64_to_cpu(super->data_offset) + __le64_to_cpu(super->data_size)

quoted

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.  

I don't think we reached a consensus.  I probably got distracted.
I don't like that suggestion from Mariusz as it makes assumptions that I
didn't want to make.  I think it is safest to always test dsize against
bother ->size and ->data_size without baking in assumptions about when
either is meaningful.

Hi Neil,
It seems that I failed to understand it again. You are right, you approach is
safer. Please fix stylistic issues then and I'm fine with the change.

Sorry for confusing you,
Mariusz