[flashrom] [PATCH] fix output of erase_and_write_flash and surroundings (was: Fix out-of-bounds access if all erase functions fail)

[Stefan Tauner]

On Thu, 21 Jul 2011 03:08:36 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 21.07.2011 02:41 schrieb Carl-Daniel Hailfinger:
> > Here's the fix with no message changes. I think that part is where we
> > both agree. Please note that a separate followup patch with improved
> > messages (either from you or from me) is also very desirable for 0.9.4,
> > and by now I pretty much agree with your reasoning.
> >   
> 
> And here are the pure message changes on top.
> AFAICS they should be mostly what you created yourself and a few small
> tweaks by me, so it should probably carry your signoff. In the meantime,
> this has my signoff to make sure nobody thinks the patch is restricted.
> 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
> 
> --- flashrom-cosmetics_blockwalker_read_write_error/flashrom.c	2011-07-21 02:50:59.000000000 +0200
> +++ flashrom-cosmetics_blockwalker_read_write_error/flashrom.c	2011-07-21 03:00:20.000000000 +0200
> @@ -1526,15 +1526,14 @@
>  			msg_cdbg("No usable erase functions left.\n");
>  			break;
>  		}
> -		msg_cdbg("Looking at blockwise erase function %i... ", k);
> +		msg_cdbg("Trying erase function %i... ", k);
>  		if (check_block_eraser(flash, k, 1)) {
>  			msg_cdbg("Looking for another erase function.\n");
>  			continue;
>  		}
>  		usable_erasefunctions--;
> -		msg_cdbg("trying... ");
> -		ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents);
> -		msg_cdbg("\n");
> +		ret = walk_eraseregions(flash, k, &erase_and_write_block_helper,
> +					curcontents, newcontents);
>  		/* If everything is OK, don't try another erase function. */
>  		if (!ret)
>  			break;
> @@ -1544,14 +1543,19 @@
>  		 */
>  		if (!usable_erasefunctions)
>  			continue;
this case now is an exception: it does not print a "Looking for
another erase function" message.

a possible fix is to add this at the top of the loop:
if (k != 0)
	msg_cdbg("Looking for another erase function.\n");

> +		/* Reading the whole chip may take a while, inform the user even
> +		 * in non-verbose mode.
> +		 */
> +		msg_cinfo("Reading current flash chip contents... ");
>  		if (flash->read(flash, curcontents, 0, size)) {
>  			/* Now we are truly screwed. Read failed as well. */
> -			msg_cerr("Can't read anymore!\n");
> +			msg_cerr("Can't read anymore! Aborting.\n");
>  			/* We have no idea about the flash chip contents, so
>  			 * retrying with another erase function is pointless.
>  			 */
>  			break;
>  		}
> +		msg_cinfo("done. Trying next erase function.\n");

i am quoting you:
> msg_cdbg("Looking for another erase function.\n");
> That would make the messages more consistent.

and you are mixing dbg with info in your version.
the done should be info, the trying thingy dbg. i almost missed that
too... this loop is a hard nut :)

>  	}
>  	/* Free the scratchpad. */
>  	free(curcontents);
> @@ -1938,13 +1942,13 @@
>  	 * preserved, but in that case we might perform unneeded erase which
>  	 * takes time as well.
>  	 */
> -	msg_cdbg("Reading old flash chip contents... ");
> +	msg_cinfo("Reading old flash chip contents... ");
>  	if (flash->read(flash, oldcontents, 0, size)) {
>  		ret = 1;
> -		msg_cdbg("FAILED.\n");
> +		msg_cinfo("FAILED.\n");
>  		goto out;
>  	}
> -	msg_cdbg("done.\n");
> +	msg_cinfo("done.\n");
>  
>  	// This should be moved into each flash part's code to do it 
>  	// cleanly. This does the job.
> 

i have fixed those issues in the attached patch and below are the
dummy-simulated outputs of writes and erases.

verbose output with one error:
Reading old flash chip contents... done.
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EBLOCK ERASE 0xd8 fails because i said so!
Invalid command sent to flash chip!
spi_block_erase_d8 failed during command execution at address 0x0

Reading current flash chip contents... done. Looking for another erase function.
Trying erase function 1... 0x000000-0x01ffff:EW
Done.
Verifying flash... VERIFIED.  

non-verbose output with one error:
Reading old flash chip contents... done.
Erasing and writing flash chip... Invalid command sent to flash chip!
spi_block_erase_d8 failed during command execution at address 0x0
Reading current flash chip contents... done. Done.
Verifying flash... VERIFIED.

verbose output without errors:
Reading old flash chip contents... done.
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EW, 0x008000-0x00ffff:EW, 0x010000-0x017fff:EW, 0x018000-0x01ffff:EW
Done.
Verifying flash... VERIFIED.

new non-verbose output without errors:
Reading old flash chip contents... done.
Erasing and writing flash chip... Done.
Verifying flash... VERIFIED.


non-verbose erase with one error:
Erasing and writing flash chip... Invalid command sent to flash chip!
spi_block_erase_d8 failed during command execution at address 0x0
Reading current flash chip contents... done. Done.

^ that one is a bit odd, because we dont tell the use why we do it and
it is not obvious. OTOH one could argue that this is caused by sharing
the erase_and_write_flash function as is. any ideas how this could be
fixed?

another thing that is obvious in the case above but applies to all: the
D in Done should be d (or vice-versa).

non-verbose erase with no errors:
Erasing and writing flash chip... Done.

verbose erase with one error:
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:EBLOCK ERASE 0xd8 fails because i said so!
Invalid command sent to flash chip!
spi_block_erase_d8 failed during command execution at address 0x0

Reading current flash chip contents... done. Looking for another erase function.
Trying erase function 1... 0x000000-0x01ffff:E
Done.

verbose erase with no errors:
Erasing and writing flash chip... Trying erase function 0... 0x000000-0x007fff:E, 0x008000-0x00ffff:E, 0x010000-0x017fff:E, 0x018000-0x01ffff:E
Done.

we should also get rid of that extra \n in the case of errors.
i think it is the one in walk_eraseregions in the if that checks
do_something. this breaks consistency in that function, but because
some/most/all(?) error messages in erasers have a line break at the end
this is the right thing to do. i have commented it out in the attached
patch (the outputs above are with it still included), so you can spot it
easily.

in general i think this is a step forward and i am glad that all those
"invested hours" show some return ;)
regarding the sign-off... i dont really care, but the best solution is
probably, that we both sign it... we actually have done that already for
"our" parts respectively. i dont see a problem with that, but i am fine
with any other solution too.

todo:
 - \n
 - Done
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-output-of-erase_and_write_flash-and-surroundings.patch
Type: text/x-patch
Size: 3297 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110721/1b1bf5a1/attachment.patch>