Am 20.07.2011 16:20 schrieb Stefan Tauner:
carldani: if you dont wanna ready anything about output printing skip to the paragraph/chunk before the last ;)
Thanks for the info. I thought about your suggested output changes for a long time and now I'm convinced that your point is valid. We should reduce output elsewhere, though.
On Mon, 18 Jul 2011 02:34:57 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 17.07.2011 15:46 schrieb Stefan Tauner:
On Sun, 17 Jul 2011 15:20:59 +0200 Carl-Daniel Hailfinger wrote:
Am 16.07.2011 23:44 schrieb Stefan Tauner:
On Sat, 16 Jul 2011 02:13:44 +0200 Carl-Daniel Hailfinger wrote:
> Am 16.04.2011 13:37 schrieb Stefan Tauner: > >
[holes eraser array] Where did you find holes in that array? If a patch contains flash chip definitions with holes, it should be rejected.
i did not find one in flashchips.c, but it is possible to introduce them via the probing function theoretically. should not happen, will probably never happen, would guard against it anyway if possible. :)
We should guard against that in a selfcheck function on startup, not at write/erase time.
we can't guard against a chip probing function that modifies the flashchip struct and introduce holes (except by making them constants). i don't think it is a big problem though... we always iterate over the whole block_erasers array so we will just skip "holes".
Indeed, a programmatically constructed struct flashchip might behave odd in various ways.
Found chip "Winbond W25Q32" (4096 kB, SPI) on serprog. Erasing and writing flash chip... ERASE FAILED! Reading current chip content... <very long pause>done. Trying next erase function. Done.
without the new "Reading current chip content..." there would be only "ERASE FAILED!" be printed for a quite long time. i think that's scary for users, and that they may hit ctrl+c because of it.
Sorry, I don't buy that argument. flashrom already emits too much information in non-verbose mode, and this is not going to improve the situation.
5 lines + header for an erase cycle where one erase function fails is too much?
Not in itself, but I doubt the messages will fit in 5 lines in the near future when someone discovers that we need more default verbosity in other places, and by then a quiet mode will be needed for flashrom.
which would be a good solution imho. i dont think that the user interface design of flashrom is appropriate because it is does a very delicate job. printing "ok" or "failed" only is not enough for something like that.
We spent weeks trying to determine the correct message levels back when we migrated everything from printf to msg_*. That said, an audit of the current state would indeed be a good idea, but the biggest hurdle will be finding someone willing to review any changes.
If people freak out, there is a good chance they will freak out already during write, and won't wait for the scary "ERASE FAILED" error message.
during write there the last characters are "..." not "FAILED". i hope we can agree that this is a major difference in the perspective of a user :) at least for me "..." indicates "it does something which might take a while, dont worry".
I am unhappy about the inconsistency this creates for messages about reading the flash chip. Depending on the flash chip and the operation, flashrom may read the full flash chip 8 times: Once during startup, once for each failed erase method (up to 6), once for final verify. Individual write methods may have internal verify/read steps as well. Only reads after a failed erase would be printed at default verbosity, and that inconsistency worries me.
that is very true. my main concerns are long waits where there is no output and where it is not clear that we did not hang. so the short reads for erase verification etc do not worry me. the one at startup should be announced though imo.
Absolutely, yes.
You once said that you think flashrom floods the user with too much output. Did you change your mind?
that was *only* about ichspi in verbose mode. for non-verbose mode i think it could inform the user more about what it does in general (alternative would be to have at least a progress bar.. i know wip).
I think the Chromium branch of flashrom has a totally different user experience because it changed lots of output levels. We might want to take a look at their approach and check if it fits our needs as well.
first we need to discuss what our needs are :)
Agreed.
./flashrom -pserprog:dev=/dev/ttyU2flasher:115200 -c W25Q32 -EV [...] Erasing and writing flash chip... Looking at erase function 0... looks ok. Trying... 0x000000-0x000fff:E ERASE FAILED!
Reading current chip content... <very long pause>done. Trying next erase function. Looking at erase function 1... block erase function found, but eraseblock layout is not defined. Looking for another erase function. Looking at erase function 2... eraseblock layout is known, but matching block erase function is not implemented. Looking for another erase function. Looking at erase function 3... not defined. Looking for another erase function. Looking at erase function 4... looks ok. Trying... 0x000000-0x3fffff:S Done.
nice verbose output explaining what really happens.. the extra line break is from inside the erase_and_write_block_helper i think.
From: Stefan Tauner stefan.tauner@student.tuwien.ac.at Date: Sat, 16 Jul 2011 22:53:24 +0200 Subject: [PATCH 1/2] Fix out-of-bounds access if all erase functions fail
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
flashrom.c | 29 +++++++++++++++++++---------- 1 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/flashrom.c b/flashrom.c index 998a18f..25cd43e 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1502,6 +1502,8 @@ static int check_block_eraser(const struct flashchip *flash, int k, int log) "eraseblock layout is not defined. "); return 1; }
- if (log)
msg_cdbg("looks ok. ");Unneeded verbosity.
i beg to differ. i think it makes the verbose output more readable for someone who does not know the code (well). i think there should always be a terminating print for messages ending with "..." like this one. maybe just "ok."? :)
Unless I'm mistaken, the original "trying..." message fulfilled exactly that purpose.
partially. i think having dedicated opening and closing "terminals" around ellipses are better, because it is a pattern known by the users. the "trying..." message does not match that pattern exactly because it combines the closing terminal for "Looking at erase function x... " with a new opening terminal. also, it moves the output away from the printing of the closing terminals done in cases of errors. that's all a bit inconsistent in my view. but you are right that the trying message somewhat solves the basic need for a closing terminal.
"Trying" instead of "Looking at" sounds good.
return 0; }
@@ -1522,34 +1524,41 @@ int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t memcpy(curcontents, oldcontents, size);
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) {
msg_cdbg("Looking at blockwise erase function %i... ", k);if (check_block_eraser(flash, k, 1) && usable_erasefunctions) {
if (!usable_erasefunctions) {msg_cdbg("No usable erase functions left.\n");break;}msg_cdbg("Looking at erase function %i... ", k);if (check_block_eraser(flash, k, 1)) { msg_cdbg("Looking for another erase function.\n"); continue;
msg_cdbg("trying... ");ret = walk_eraseregions(flash, k, &erase_and_write_block_helper, curcontents, newcontents);msg_cdbg("\n");
msg_cdbg("Trying... ");Why did you change trying->Trying? Consistency?
to have correct output together with the "looks ok." print above: "Looking at erase function 4... looks ok. Trying..." vs. "Looking at erase function 4... looks ok. trying..."
Compare that to the current message:
"Looking at erase function 4... trying..."
The current message does not mislead users to assume that the erase function is OK. We will only know that after it has been tried, and the user does not care about flashrom internals.
well, if you dont want the user to know about it, then it might be better to not even tell him that we are looking for well-defined erasers, but just that we are trying them. "Trying erase function x..." instead of the "Looking at" message and droping the "trying..." message altogether. the matching closing term would then be either one of the failure messages of check_block_eraser or the "ERASE FAILED!" of erase_and_write_block_helper. and.. there we are missing another closing terminal for successful runs. this is something i noticed to cause confusion too (at least for me :)
Mh. First and foremost I care about the output of non-verbose mode. That should be reasonably sane. Second priority is verbose mode output. Spew output is expected to be an unreadable badly formatted flood of mostly useless messages.
My main objection to this patch is that it combines a bugfix with cosmetics, and the cosmetics distract from bugfixing. The bugfix part (and the ret=1 initialization you mentioned) would have been easily reviewable/ackable, yet we're investing hours into dicussing how the output should look like.
true, this should not be mixed up, i am sorry. we should get the fix in and discuss the rest later when you have more time again (i am still not giving up hope :)
Heh.
Here is my take on the bugfix part with minimal message changes:
i dunno if you are super busy atm or you just missed my private messages on irc, but you forgot to attach a patch...
Sorry, I was not on IRC for the last few days, and now I'm trying to catch up on mail first, then move on to IRC.
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.
Fix out-of-bounds access if all erase functions fail. Fix detection of unchanged chip contents on erase failure. Return error if no usable erase functions exist.
Thanks to Stefan Tauner for spotting the last problem.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-fix_erasefunctions_nullpointer/flashrom.c =================================================================== --- flashrom-fix_erasefunctions_nullpointer/flashrom.c (Revision 1373) +++ flashrom-fix_erasefunctions_nullpointer/flashrom.c (Arbeitskopie) @@ -1507,7 +1507,7 @@
int erase_and_write_flash(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents) { - int k, ret = 0; + int k, ret = 1; uint8_t *curcontents; unsigned long size = flash->total_size * 1024; unsigned int usable_erasefunctions = count_usable_erasers(flash); @@ -1522,8 +1522,12 @@ memcpy(curcontents, oldcontents, size);
for (k = 0; k < NUM_ERASEFUNCTIONS; k++) { + if (!usable_erasefunctions) { + msg_cdbg("No usable erase functions left.\n"); + break; + } msg_cdbg("Looking at blockwise erase function %i... ", k); - if (check_block_eraser(flash, k, 1) && usable_erasefunctions) { + if (check_block_eraser(flash, k, 1)) { msg_cdbg("Looking for another erase function.\n"); continue; } @@ -1535,10 +1539,8 @@ if (!ret) break; /* Write/erase failed, so try to find out what the current chip - * contents are. If no usable erase functions remain, we could - * abort the loop instead of continuing, the effect is the same. - * The only difference is whether the reason for other unusable - * functions is printed or not. If in doubt, verbosity wins. + * contents are. If no usable erase functions remain, we can + * skip this: the next iteration will break immediately anyway. */ if (!usable_erasefunctions) continue;