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.

View Change

4 comments:

• File flashrom.c:

  • Patch Set #6, Line 2396: msg_cinfo("Assumed old flash chip contents as ref-file...\n");

    Nit, maybe rather "Assuming ..."?

  • Patch Set #6, 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.

  • Patch Set #6, 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.

  • Patch Set #6, 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).

To view, visit change 23263. To unsubscribe, or for help writing mail filters, visit settings.