[flashrom] [PATCH] fix output of erase_and_write_flash and surroundings (was: Fix out-of-bounds access if all erase functions fail)
Stefan Tauner
stefan.tauner at student.tuwien.ac.at
Thu Jul 21 18:36:59 CEST 2011
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>
More information about the flashrom
mailing list