On Thu, 25 Aug 2022 10:24:21 +1000
"NeilBrown" <neilb@xxxxxxx> 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@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.
>
> 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
