On 10.06.2010 16:33, Michael Karcher wrote:
> Am Dienstag, den 01.06.2010, 04:54 +0200 schrieb Carl-Daniel Hailfinger:
>
>> -int read_flash(struct flashchip *flash, char *filename);
>> +int read_flash_write_file(struct flashchip *flash, char *filename);
>>
> Cumbersome name. Use "read_flash_to_file".
>
Done.
>> +int read_flash_write_file(struct flashchip *flash, char *filename)
>> +{
>> + unsigned long size = flash->total_size * 1024;
>> + unsigned char *buf = calloc(size, sizeof(char));
>> +
>> + msg_cinfo("Reading flash... ");
>> + if (!flash->read) {
>> + msg_cinfo("FAILED!\n");
>> + msg_cerr("ERROR: flashrom has no read function for this flash
>> chip.\n");
>> + return 1;
>> + }
>> + if (flash->read(flash, buf, 0, size))
>> + return 1;
>>
> I know you didn't change that code. Do we really want no error message
> here?
>
Fixed.
I moved the code listed above to a separate patch:
[PATCH] Refactor/fix read-to-file
>> + return write_buf_to_file(buf, flash->total_size * 1024, filename);
>> +}
>>
>
>
>
>> +void nonfatal_help_message(void)
>> +{
>> + msg_gerr("Writing to the flash chip apparently didn't do anything.\n"
>>
> "Trying to write to the flash chip..."
>
Hm. An earlier message says "writing", not "trying to write". With your
change, we'd see messages like this:
Writing flash chip...
Trying to write to the flash chip apparently didn't do anything.
That looks odd. I agree that my proposed message wasn't really good
style, but the replacement isn't perfect either. Maybe a native speaker
can comment.
>> + "This means we have to add special support for your board, "
>> + "programmer or flash chip.\n"
>> + "Please report this on IRC at irc.freenode.net (channel "
>> + "#flashrom) or\n"
>> + "mail [email protected]!\n"
>> + "-------------------------------------------------------------"
>> + "------------------\n"
>> + "You may now reboot or simply leave the machine running.\n");
>>
> "As nothing changed, powering off or rebooting the machine is not
> dangerous".
>
Should this message be printed in addition to or as replacement for "You
may now reboot..."?
How should we tell the user that rebooting is unsafe if a previous write
failed?
Example scenario: Chip is filled with data, user tries to write, but
write fails and the chip is now erased. We print a scary warning. User
retries, the write fails again but this time nothing changed (the chip
was already erased), so we print the non-scary warning. The user is
confused and reboots. Boom!
>> - msg_cerr("FAILED!\n");
>> + msg_cerr("Uh oh. Write failed. Checking if anything "
>> + "changed.\n");
>> + /* FIXME: Should we really reuse buf here? */
>> + if (!flash->read(flash, buf, 0, size)) {
>> + if (!memcmp(oldflashcontents, buf, size)) {
>> + msg_cinfo("Good. It seems nothing was "
>> + "changed.\n");
>> + nonfatal_help_message();
>> + programmer_shutdown();
>> + return 1;
>> + }
>> + }
>>
> Wait a moment... How can the chip be unmodified after passing an erase
> check? Only if it was empty before - no need to panic in that case. I
> suggest to remove the "did anything change" check from this piece of
> code.
>
What about a partially failed write? If the chip was erased before, the
erase check will pass. If write fails completely, the chip is still
erased. If write fails partially, you have a partially written chip.
Total write failure is not a problem with already erased chips, but
partial write failure is indeed a problem.
Did I understand your thoughts correctly or did you mean something else?
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom
