Attention is currently required from: Simon Buhrow, Nico Huber, Aarya, Anastasia Klimchuk.
10 comments:
Commit Message:
flashrom.c: Add wrapper function to use the erase algorithm
Add a function to call the erase algorithm.
commit message is unrelated to content.
File flashrom.c:
Patch Set #72, Line 1349: static int erase_by_layout
It would be my view to more your whole implementation into its own separate file. […]
If it is easier to not have your work sitting in review forever and you want to put everything initially in flashrom.c I would suggest the following pattern for the minimal amount of reshuffling for you:
i) Create some trampoline functions that will dispatch into your algorithm and a few if branches to jump to them. Use a static bool global `use_legacy_erase_path = true;` and branch off that.
ii) Adding all the new functions in one go atomically without the need for the prototypes hacks.
iii) Have a small commit that toggles `use_legacy_erase_path` from `true->false`.
So for step (i), for this function in another commit rename it as `erase_by_layout_legacy()` and call it with:
```
static int erase_by_layout(struct flashctx *const flashctx)
{
if (use_legacy_erase_path)
return erase_by_layout_legacy(flashctx);
}
```
Subsequently for step (ii) you can do:
```
static int erase_by_layout(struct flashctx *const flashctx)
{
if (use_legacy_erase_path)
return erase_by_layout_legacy(flashctx);
return erase_by_layout_optimised(flashctx);
}
```
File flashrom.c:
Patch Set #78, Line 720: <= len
what is happening here? Seems like its own change but not what the commit says "add wrapping function".
Patch Set #78, Line 1374: size_t
`const uint32_t`, A `size_t` is a a system specific type.
const
Patch Set #78, Line 1382: curcontents == NULL || newcontents == NULL
`!ptr` is canonical. Also `erase_layout` never checked.
Patch Set #78, Line 1422: write_by_layout
in a separate commit before this one, rename as `write_by_layout_legacy()` and,
```
static int write_by_layout(struct flashctx *const flashctx,
uint8_t *const curcontents,
const uint8_t *const newcontents)
{
if (use_legacy_erase_path)
return write_by_layout_legacy(flashctx, curcontents, newcontents);
}
```
then you can put your new implementation in the above in this commit.
const struct flashrom_layout *const flash_layout = get_layout(flashctx);
const struct romentry *entry = NULL;
struct erase_layout *erase_layout = create_erase_layout(flashctx);
move below `erasefn_count` and `ret`
Patch Set #78, Line 1427: erase_layout
check for nullarity?
`const`
To view, visit change 66104. To unsubscribe, or for help writing mail filters, visit settings.