Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk.

View Change

6 comments:

• Patchset:

  • Patch Set #22:

    Passing an abstract configuration structure to WP functions is probably a better option than the int […]

    Sorry for the silence. I think you made good progress on your own! The abstract
    struct leaves the function signatures nice and simple. However, exposed structs
    form an ABI along with potential trouble (see inline comment). It might be better
    to have an opaque struct with getters and setters, e.g.

    ```
    enum fashrom_wp_mode {
            WP_MODE_DISABLE,
            WP_MODE_HARDWARE,
    };
    struct flashrom_wp_cfg;

    int flashrom_wp_cfg_new(struct flashrom_wp_cfg **);
    void flashrom_wp_cfg_release(struct flashrom_wp_cfg *);
    void flashrom_wp_set_mode(struct flashrom_wp_cfg *, enum flashrom_wp_mode);
    enum flashrom_wp_mode flashrom_wp_get_mode(const struct flashrom_wp_cfg *);
    void flashrom_wp_set_range(struct flashrom_wp_cfg *, size_t start, size_t len);
    void flashrom_wp_get_range(size_t *start, size_t *len, const struct flashrom_wp_cfg *);

    int flashrom_wp_get_cfg(struct flashrom_wp_cfg **, struct flashrom_flashctx *);
    int flashrom_wp_set_cfg(struct flashrom_flashctx *, const struct flashrom_wp_cfg *);
    ```

    This would allow us to extend the API in the future if necessary without
    touching any of the lines above. WDYT?

• File libflashrom.h:

  • Patch Set #27, Line 133: };

    Having a struct in the API means we can't ever change the struct without
    versioning the ABI. Beside that I don't know the mechanisms behind the
    latter I think it's not necessary. For instance, we could have an opaque
    object like the others, and getters and setters for mode and range, WDYT?

  • Patch Set #27, Line 138: #define FLASHROM_WP_ERR_VERIFY_FAILED 4

    This could be an enum, maybe even without specific numbers?

• File writeprotect.c:

  • Patch Set #22, Line 63: *cfg = (struct wp_chip_config) {0};

    I think I might have relied on this in an earlier patch set but I don't remember now. […]

    Done

• File writeprotect.c:

  • Patch Set #27, Line 118: }

    Maybe drop braces here for concistency.

  • Patch Set #27, Line 161:

    to write
     *                      to the chip
    

    drop

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