Attention is currently required from: Anastasia Klimchuk, Daniel Campello, Edward O'Callaghan, Nico Huber, Richard Hughes.

View Change

1 comment:

• File tests/chip.c:

  • Patch Set #6, Line 86: struct progress_user_data *progress_user_data = progress_state->user_data;

    I hadn't noticed the `user_data` field, which makes `const` more challenging.

    Since `flashrom_set_progress_callback` initializes `flashctx.progress_state` to a user-provided pointer, it seems like the caller is meant to retain ownership of the `progress_state` in which case a non-const pointer seems more correct (consider the case of a caller `malloc`ing the progress struct: they would then need to cast the `const` away to `free` it before tearing down the flash context). A user might also want to update the `user_data` pointer (which they own) during a progress callback, which `const` would forbid.

    It seems like the way `progress_state` needs to be owned by the caller is a mistake, because that actually contains state owned by flashrom which should be immutable for users (reporting the current progress) and also contains mutable state owned by users. Redefining the callback would be ideal (`void(flashrom_progress_callback)(enum flashrom_progress stage, size_t, size_t, void*)`) instead, but in the absence of a larger API change I think the returned state pointer needs to remain mutable.

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