Re: [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 [e-mail]!\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