On Fri, 2 Jun 2017 21:45:37 +0300, Andy Shevchenko wrote: > On Fri, Jun 2, 2017 at 9:40 PM, Jean Delvare <jdelv...@suse.de> wrote: > > On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote: > >> Your commit message should answer to the question why and what. > >> You didn't put it there. > >> Moreover, the change above per se doesn't belong to this — one logical > >> change per patch. > > > > I'm confused. These changes totally belong to this patch. They belong > > so much to it, that's the very reason why they are not described > > separately in the commit message. > > > > The purpose of the patch is to check that the records are large enough > > to contain the fields we need to access. Setting a pointer beyond the > > end of the record _before_ performing that check makes no sense. > > > > I did not include these changes as performance optimizations, I > > included them because they make the code conceptually correct. It's > > even clearer for the last instance, as we are dereferencing the pointer > > immediately, but in my opinion, even setting a pointer to a location > > which may not exist is equally wrong and confusing for the reader. > > That's why I moved that code after the length checks. > > You are talking here explicitly about third case which I agreed on. > > The two first ones are not the same. > You didn't dereference them before check since your check is not > against pointer. > > So, basically it means you are checking pointer _indirectly_.
Correct. > I think we already spent too much time on this one. Agreed. > If you wish to leave your changes, update commit message accordingly. No. -- Jean Delvare SUSE L3 Support