[flashrom-gerrit] [M] Change in flashrom[main]: libflashrom: Update the API for progress callback

[Anastasia Klimchuk (Code Review)]

Attention is currently required from: Peter Marheine, Richard Hughes, Sergii Dmytruk.

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/86031?usp=email )

Change subject: libflashrom: Update the API for progress callback
......................................................................

Patch Set 9:

(2 comments)

File doc/release_notes/devel.rst:

https://review.coreboot.org/c/flashrom/+/86031/comment/277f4856_d055fd48?usp... :
PS8, Line 39: New libflashrom API for progress callback
            : -----------------------------------------
            : 
            : API for progress callback has changed. Old one ``flashrom_set_progress_callback`` is deprecated.
            : New API ``flashrom_set_progress_callback_v2`` should be used instead.
            : 
            : The type of callback function has changed as well, old function type ``flashrom_progress_callback``
            : is deprecated, new function type ``flashrom_progress_callback_v2`` should be used instead.
            : 
            : The reason for deprecation is that old API for progress indicator was possible to use only via the hack
            : (like client of API keeping the pointer in a global state), and to allow the normal usage of API a change
            : was needed one way or the other.
> Rewording to read better, I think. […]

Thank you , done! I only added once sentence about `flashrom_progress_callback_v2` on top of your text.

File include/flash.h:

https://review.coreboot.org/c/flashrom/+/86031/comment/b8925f9c_08af3fcf?usp... :
PS8, Line 624: 	flashrom_progress_callback_v2 *progress_callback;
             : 	struct flashrom_progress progress_state;
             : 	struct stage_progress stage_progress[FLASHROM_PROGRESS_NR];
             : 	/* deprecated, do not use */
             : 	flashrom_progress_callback *deprecated_progress_callback;
             : 	struct flashrom_progress *deprecated_progress_state;
> Could you make this a tagged union, so it's more obviously one or the other and never both? Since th […]

There is one important detail in current implementation which I think won't work with the union.

But tell me if I am missing something.

The old API was exposing progress_state to the client of API, and the client owned the pointer. Which was one of the things to fix in the new API, so client no longer has the pointer to progress_state, only pointer to user_data.

So now we keep both versions of API working, and available for users. But, to avoid exposing progress_state to the clients (and to avoid allowing them to modify progress_state outside of internal flashrom logic), deprecated API is using its copy of progress_state.
Every time deprecated callback is called, real progress_state is copied to deprecated progress state and with this even if the client is using deprecated API they still can't modify the real internal progress state.

This is also in a thread in this patch, about this :)

But in short, even if old API is used, both old and new progress_state are used, so they can't be in a union.

On a long term, as I understand, that old API can be removed in v2.0, and I will certainly do it (remove the old API).