Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/70894 )
Change subject: layout,libflashrom: Split layout data creation from ADT ......................................................................
layout,libflashrom: Split layout data creation from ADT
The blending of the layout ADT implementation detail with the contained data makes it awkward to deal with. Split the creation and initalisation of the layout data from that of the layout LL implementation.
Change-Id: Id989cbb5efbbdc03200ca472b59db2bd03779877 Signed-off-by: Edward O'Callaghan quasisec@google.com --- M flashrom.c M ich_descriptors.c M include/libflashrom.h M layout.c M libflashrom.c M tests/chip.c M tests/chip_wp.c M tests/layout.c 8 files changed, 99 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/94/70894/1
diff --git a/flashrom.c b/flashrom.c index 2fb532e..9739e9a 100644 --- a/flashrom.c +++ b/flashrom.c @@ -821,7 +821,8 @@ /* Fill default layout covering the whole chip. */ if (flashrom_layout_new(&flash->default_layout) || flashrom_layout_add_region(flash->default_layout, - 0, flash->chip->total_size * 1024 - 1, "complete flash") || + flashrom_layout_create_region( + 0, flash->chip->total_size * 1024 - 1, "complete flash")) || flashrom_layout_include_region(flash->default_layout, "complete flash")) return -1; return 0; diff --git a/ich_descriptors.c b/ich_descriptors.c index b37ce6d..b2d0b9c 100644 --- a/ich_descriptors.c +++ b/ich_descriptors.c @@ -1415,7 +1415,8 @@ const chipoff_t limit = ICH_FREG_LIMIT(desc.region.FLREGs[i]); if (limit <= base) continue; - if (flashrom_layout_add_region(*layout, base, limit, regions[i])) { + const struct flash_region *region = flashrom_layout_create_region(base, limit, regions[i]); + if (flashrom_layout_add_region(*layout, region)) { flashrom_layout_release(*layout); *layout = NULL; return 2; diff --git a/include/libflashrom.h b/include/libflashrom.h index bac76c2..4b1c3d0 100644 --- a/include/libflashrom.h +++ b/include/libflashrom.h @@ -397,17 +397,26 @@ int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **layout, struct flashrom_flashctx *flashctx, const uint8_t *buffer, size_t len); /** - * @brief Add a region to an existing layout. + * @brief Create a region to add to layout. * - * @param layout The existing layout. * @param start Start address of the region. * @param end End address (inclusive) of the region. * @param name Name of the region. * + * @return flash_region on success, + * NULL if out of memory. + */ +struct flash_region *flashrom_layout_create_region(const unsigned int start, const unsigned int end, const char *const name); +/** + * @brief Add a region to an existing layout. + * + * @param layout The existing layout. + * @param region The region to add. + * * @return 0 on success, * 1 if out of memory. */ -int flashrom_layout_add_region(struct flashrom_layout *layout, size_t start, size_t end, const char *name); +int flashrom_layout_add_region(struct flashrom_layout *const layout, const struct flash_region *region); /** * @brief Mark given region as included. * diff --git a/layout.c b/layout.c index f2973d1..6ad587b 100644 --- a/layout.c +++ b/layout.c @@ -102,9 +102,13 @@ msg_gerr("Error parsing layout file. Offending string: "%s"\n", tempstr); goto _close_ret; } - if (flashrom_layout_add_region(*layout, - strtol(tstr1, NULL, 16), strtol(tstr2, NULL, 16), tempname)) + + struct flash_region *region = flashrom_layout_create_region( + strtol(tstr1, NULL, 16), strtol(tstr2, NULL, 16), tempname); + if (flashrom_layout_add_region(*layout, region)) { + free(region); goto _close_ret; + } } ret = 0;
@@ -401,36 +405,46 @@ return 0; }
-int flashrom_layout_add_region( - struct flashrom_layout *const layout, - const size_t start, const size_t end, const char *const name) +struct flash_region *flashrom_layout_create_region( + const unsigned int start, const unsigned int end, const char *const name) { + char *rname = strdup(name); + if (!rname) + return NULL; + + struct flash_region *region = calloc(1, sizeof(*region)); + if (!region) { + free(rname); + return NULL; + } + + region->start = start; + region->end = end; + region->name = rname; + + return region; +} + +int flashrom_layout_add_region( + struct flashrom_layout *const layout, const struct flash_region *region) +{ + if (!region) + return 1; + struct romentry *const entry = malloc(sizeof(*entry)); - if (!entry) - goto _err_ret; + if (!entry) { + msg_gerr("Error adding layout entry: %s\n", strerror(errno)); + return 1; + }
- const struct romentry tmp = { - .next = layout->head, - .included = false, - .file = NULL, - .region = { - .start = start, - .end = end, - .name = strdup(name), - }, - }; - *entry = tmp; - if (!entry->region.name) - goto _err_ret; + memcpy(&entry->region, region, sizeof(*region)); + entry->next = layout->head;
- msg_gdbg("Added layout entry %08zx - %08zx named %s\n", start, end, name); layout->head = entry; - return 0; + msg_gdbg("Added layout entry %08x - %08x named %s\n", + region->start, region->end, region->name);
-_err_ret: - msg_gerr("Error adding layout entry: %s\n", strerror(errno)); - free(entry); - return 1; + return 0; }
int flashrom_layout_include_region(struct flashrom_layout *const layout, const char *name) diff --git a/libflashrom.c b/libflashrom.c index 44f297f..b806611 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -359,7 +359,10 @@
for (i = 0, area = fmap->areas; i < fmap->nareas; i++, area++) { snprintf(name, sizeof(name), "%s", area->name); - if (flashrom_layout_add_region(l, area->offset, area->offset + area->size - 1, name)) { + struct flash_region *region = flashrom_layout_create_region( + area->offset, area->offset + area->size - 1, name); + if (flashrom_layout_add_region(l, region)) { + free(region); flashrom_layout_release(l); return 1; } diff --git a/tests/chip.c b/tests/chip.c index 1d9cdc9..9ec5311 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -115,7 +115,8 @@ printf("Creating layout with one included region... "); assert_int_equal(0, flashrom_layout_new(layout)); /* One region which covers total size of chip. */ - assert_int_equal(0, flashrom_layout_add_region(*layout, 0, chip->total_size * KiB - 1, "region")); + assert_int_equal(0, flashrom_layout_add_region(*layout, + flashrom_layout_create_region(0, chip->total_size * KiB - 1, "region"))); assert_int_equal(0, flashrom_layout_include_region(*layout, "region"));
flashrom_layout_set(flashctx, *layout); diff --git a/tests/chip_wp.c b/tests/chip_wp.c index 40303ff..389ddc2 100644 --- a/tests/chip_wp.c +++ b/tests/chip_wp.c @@ -41,8 +41,10 @@ const size_t tail_len = chip->total_size * KiB - 1;
assert_int_equal(0, flashrom_layout_new(layout)); - assert_int_equal(0, flashrom_layout_add_region(*layout, 0, tail_start - 1, "head")); - assert_int_equal(0, flashrom_layout_add_region(*layout, tail_start, tail_len, "tail")); + assert_int_equal(0, flashrom_layout_add_region(*layout, + flashrom_layout_create_region(0, tail_start - 1, "head"))); + assert_int_equal(0, flashrom_layout_add_region(*layout, + flashrom_layout_create_region(tail_start, tail_len, "tail")));
flashrom_layout_set(flash, *layout); } diff --git a/tests/layout.c b/tests/layout.c index e71debf..b91ac5d 100644 --- a/tests/layout.c +++ b/tests/layout.c @@ -31,12 +31,14 @@ printf("done\n");
printf("Adding and including first region... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00021000, 0x00031000, "first region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00021000, 0x00031000, "first region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "first region")); printf("done");
printf(", second (non-overlapping) region... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00031001, 0x0023efc0, "second region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00031001, 0x0023efc0, "second region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "second region")); printf("done\n");
@@ -59,12 +61,14 @@ printf("done\n");
printf("Adding and including first region... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00021000, 0x00031000, "first region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00021000, 0x00031000, "first region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "first region")); printf("done");
printf(", second (overlapping) region... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00027100, 0x0023efc0, "second region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00027100, 0x0023efc0, "second region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "second region")); printf("done\n");
@@ -87,12 +91,14 @@ printf("done\n");
printf("Adding and including first region... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00021000, 0x00031000, "first region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00021000, 0x00031000, "first region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "first region")); printf("done");
printf(", second (overlapping) region, not included... "); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00027100, 0x0023efc0, "second region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00027100, 0x0023efc0, "second region"))); printf("done\n");
printf("Asserting included regions do not overlap... "); @@ -117,7 +123,8 @@
printf("Creating layout with one included region... "); assert_int_equal(0, flashrom_layout_new(&layout)); - assert_int_equal(0, flashrom_layout_add_region(layout, region_start, region_end, "region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(region_start, region_end, "region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "region")); printf("done\n");
@@ -153,7 +160,8 @@
printf("Creating layout with one included region... "); assert_int_equal(0, flashrom_layout_new(&layout)); - assert_int_equal(0, flashrom_layout_add_region(layout, 0x60000000, 0x70000000, "region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x60000000, 0x70000000, "region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "region")); printf("done\n");
@@ -185,7 +193,8 @@ printf("Creating layout with one included region... "); assert_int_equal(0, flashrom_layout_new(&layout)); /* Make sure address range of region is not positive i.e. start > end. */ - assert_int_equal(0, flashrom_layout_add_region(layout, 0x00000020, 0x00000010, "region")); + assert_int_equal(0, flashrom_layout_add_region(layout, + flashrom_layout_create_region(0x00000020, 0x00000010, "region"))); assert_int_equal(0, flashrom_layout_include_region(layout, "region")); printf("done\n");
-
Edward O'Callaghan (Code Review)