carldani: if you dont wanna ready anything about output printing skip to the paragraph/chunk before the last ;)
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".
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.
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.
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 :)
./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.
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 :)
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 :)
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...