Attention is currently required from: Anastasia Klimchuk, Richard Hughes.

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

View Change

2 comments:

• File doc/release_notes/devel.rst:

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

    ```suggestion
    New libflashrom API for progress reporting
    ------------------------------------------

    The old ``flashrom_set_progress_callback`` function for requesting progress updates
    during library operations is now deprecated. Users should call
    ``flashrom_set_progress_callback_v2`` instead, which also changes the signature
    of the callback function.

    This new API fixes limitations with the old one where most users would need to
    define their own global state to track progress, and it was impossible to fix that
    issue while maintaining binary compatibility without adding a new API.
    ```

• File include/flash.h:

  • Patch Set #8, 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 this is an internal implementation of a public API, I don't think it makes sense to mark these internals as deprecated because we still want to maintain compatibility.

    ```

    enum progress_kind {
        PROGRESS_V1,
        PROGRESS_V2,
    };

    struct progress_info_v1 {
        flashrom_progress_callback *callback;
        struct flashrom_progress *state;
    }

    struct progress_info_v2 {
        flashrom_progress_callback_v2 *callback;
        struct flashrom_progress state;
    }

    ...

        enum progress_kind progress_kind;
        union {
            struct progress_info_v1 progress_v1;
            struct progress_info_v2 progress_v2;
        };
    	struct stage_progress stage_progress[FLASHROM_PROGRESS_NR];
    ```

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