Re: [PATCH 1/2] mtdpart: Avoid divide-by-zero on out-of-reach path

["Jörn Engel" <joern@logfs.org>]

On Mon, 16 June 2008 23:32:22 +0900, Atsushi Nemoto wrote:
> 
>  
> -			for (i--; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
> +			i--;
> +			slave->mtd.erasesize = regions[i].erasesize;
> +			for (; i < master->numeraseregions && slave->offset + slave->mtd.size > regions[i].offset; i++) {
>  				if (slave->mtd.erasesize < regions[i].erasesize) {
>  					slave->mtd.erasesize = regions[i].erasesize;
>  				}

While this patch appears to work, I still don't like it.  Before the
patch, the whole function is simply a mess.  After your patch, it looks
even worse and becomes almost impossible to understand.  So while you
are fixing a bug today, the very next change may introduce a new bug
simply because whoever makes the change doesn't understand the code.

At least I have a hard enough time understanding it today.  The first
loop seems to look for the last eraseregion that is part of the current
partition.  Why then it should check for
	slave->offset + slave->mtd.size > regions[i].offset
instead of 
	slave->offset >= regions[i].offset

Odd.  And the second loop should go backwards as long as the
eraseregions are part of the current partition.  Which means that
	i < master->numeraseregions
doesn't make sense at all and
	slave->offset + slave->mtd.size > regions[i].offset
would imply that eraseregions go backwards.

In other words, I am tempted to replace all that with a single line:
	BUG();

At least that line is short and descriptive.  Otherwise it seems to be
roughly equivalent of what we had before.

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens