Re: [PATCH mdadm v2] super1: report truncated device

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Thu, 25 Aug 2022 10:24:21 +1000
"NeilBrown" <neilb@suse.de> wrote:

> On Thu, 25 Aug 2022, Jes Sorensen wrote:
> > On 7/25/22 03:42, Mariusz Tkaczyk wrote:  
> > > On Sat, 23 Jul 2022 14:37:11 +1000
> > > "NeilBrown" <neilb@suse.de> wrote:
> > >   
> > >> On Thu, 21 Jul 2022, Mariusz Tkaczyk wrote:  
> > >>> Hi Neil,
> > >>>
> > >>> On Wed, 13 Jul 2022 13:48:11 +1000
> > >>> "NeilBrown" <neilb@suse.de> 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.  
> 
> 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