[flashrom] [PATCH] Avoid scary warning if nothing changed

[Carl-Daniel Hailfinger]

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 flashrom at flashrom.org!\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/