[dm-devel] dm-integrity: fix inefficient allocation of stack space

[Mikulas Patocka]

On Thu, 20 Jul 2017, Mikulas Patocka wrote:

> 
> 
> On Wed, 19 Jul 2017, John Stoffel wrote:
> 
> > I'd like to argue that you should never use BUG_ON at all, esp since
> > if you have integrity running on just one critical device, but have
> > other devices that work just fine, bringing down the entire system
> > because you don't think things are ok is terrible.
> > 
> > We should rip out ALL the BUG_ONs in the dm-integrity and just do
> > proper error handling instead.  For testing, sure, use them on your
> > code.  But please, not for general use!
> > 
> > John
> 
> Linus sometimes argued against BUG_ON and people are repeating it after 
> him like sheep.
> 
> If a programmer made a bug in his code, he should fix that bug, not write 
> additional recovery code for the bug.
> 
> Mikulas

For example, there a function access_journal_check that checks if journal 
section and offset are valid and calls BUG if they're not.

Now, suppose that we change BUG() to WARN() - now we need to return error 
code from the function. So we change the return value from void to int.

But this function is called by three other functions - page_list_location, 
access_journal_entry, access_journal_data - and now we need to test the 
error code in all of them.

page_list_location returns void, so it needs to be changed to return an 
error code too. access_journal_entry and access_journal_data return a 
pointer, so we change them to return NULL on error - but that NULL needs 
to be checked in the callers.

There are 11 callers of access_journal_entry and 5 calleds of 
access_journal_data and 5 callers of page_list_location.

And now you can see that changing a single BUG to WARN adds excessive 
amount of junk code that checks for "impossible" conditions. These extra 
tests do not make the code any more readable or understandable - they make 
it less readable because the programmer no longer knows what checks are 
really needed and what are not.


In the early days when I was programming, I was adding checks for NULL 
pointers "just in case - so that it doesn't crash". But these checks 
didn't make the code any more understandable - they made the code less 
understandable - because if a function return value is checked for NULL, 
the programmer believes that the function could return NULL. And the 
programmer no longer knows what is the contract of the function - could 
the function really return NULL or not?

Mikulas