Change in flashrom[master]: Add support for reading the current flash contents from a file

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/23263 )

Change subject: Add support for reading the current flash contents from a file
......................................................................

Patch Set 6:

(4 comments)
> Sorry for double patch commits (forgot to add enum the first time)

No worries, I'm used to changes that get to 20+ patch-sets before the
review even starts ;) and we are making great progress here.

https://review.coreboot.org/#/c/23263/6/flashrom.c
File flashrom.c:

https://review.coreboot.org/#/c/23263/6/flashrom.c@2396
PS6, Line 2396: 		msg_cinfo("Assumed old flash chip contents as ref-file...\n");
Nit, maybe rather "Assuming ..."?

https://review.coreboot.org/#/c/23263/6/flashrom.c@2395
PS6, Line 2395: 	if (refcontents && oldcontents) {		
              : 		msg_cinfo("Assumed old flash chip contents as ref-file...\n");
              : 		memcpy(oldcontents, refcontents, flash_size);
              : 		memcpy(curcontents, refcontents, flash_size);
              : 	} else {
Code flow is still not correct, or at least is more limited than
it should be. IMO, like this:

if (refcontents) {
            msg_cinfo("Assumed old flash chip contents as ref-file...\n");
            memcpy(curcontents, refcontents, flash_size);
            if (oldcontents)
                    memcpy(oldcontents, refcontents, flash_size);
    }

Please read the whole function again and make sure every possible
path combination makes sense. It shouldn't be just me who sees it
as correct.

https://review.coreboot.org/#/c/23263/6/flashrom.c@2578
PS6, Line 2578: 			goto _free_ret;
Um, I shouldn't have started talking about `const` I guess. I see
what you are doing here, and like it, but now you leak the memory
behind `refcontents` here.

IMO, if you want to use `const` and a common return path, you'd
have to move the malloc() up like this:

uint8_t *const newcontents = malloc(flash_size);
    uint8_t *const refcontents = referencefile ? malloc(flash_size) : NULL;
    if (!newcontents || referencefile && !refcontents) {
            msg_gerr(...);
            goto _free_ret;
    }
    ....
    _free_ret:
    free(refcontents);
    free(newcontents);
    return ret;

But it'd also be just fine with normal pointer variables that start
with NULL and maybe point to something later.

https://review.coreboot.org/#/c/23263/6/flashrom.c@2584
PS6, Line 2584: 		ret = flashrom_image_write(flash, newcontents, flash_size, NULL);
Please always use curly braces on all paths of an if/else if you
use them on one path (it looks very unbalanced otherwise).