We are close :) though, flashrom_image_write() seems to be too
convoluted to be extended easily and needs some more attention.

View Change

7 comments:

• File cli_classic.c:

  • Patch Set #4, Line 114: C:

    You can just drop the C: and it should work. The 'C' below only
    tells getopt_long() what to return in case of --flash-contents
    which could be any distinct number. Though, better would be to
    use a non-character number like --ifd does, e.g. 0x0101. Ideally,
    we would use an enum for these numbers like this:

        enum {
                OPTION_IFD = 0x0100,
                OPTION_FLASH_CONTENTS,
        };

• File flashrom.c:

  • Patch Set #4, Line 2349: * @param refbuffer If given, assume flash contains same data as `refbuffer`

    Full stop please, as the lines above have that too.

  • Patch Set #4, Line 2349: flash

    `flash chip` please, for consistent wording.

  • Patch Set #4, Line 2357: void

    Can be (and therefore should be) const.

  • Patch Set #4, Line 2397: oldcontents = refcontents;

    I would rather copy the data here. Would allow to keep the surrounding
    code as is. You'd have to keep track of where `oldcontents` points to,
    otherwise, e.g. to make sure you don't free() `refbuffer`.

    Also, I just realized that you need to copy the data into `curcontents`
    as well. `oldcontents` is only optionally used for verification later
    and not for the checks which parts of the flash chip have to be updated.
    Basically, after this if/else block, `curcontents` has to contain valid
    data at least for all included regions in the layout, and `oldcontents`
    has to contain valid data for the whole flash if `verify_all` is set.

    With that written down, you can/should only copy into `oldcontents`
    if `verify_all` is set (in other words if `oldcontents != NULL`).

  • Patch Set #4, Line 2561: uint8_t *refcontents = NULL;

    Please move up to the general declarations above (`newcontents`
    is only declared later because it couldn't be `const` otherwise,
    stupid language if you ask me, or stupid me because I always
    try to make as much as possible `const`).

  • Patch Set #4, Line 2582: _free_ret_refcontents:

    You only need a single `_free_ret` as `free(NULL);` is valid.

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