On Thu, 21 Jul 2011 03:08:36 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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);
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);@@ -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... ");
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");
- 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