New subject: Change in flashrom[master]: layout: Introduce flashrom_layout_empty()

June 16, 2019

Hello Angel Pons, Arthur Heymans, David Hendricks, Thomas Heijligen,

I'd like you to do a code review. Please visit

    https://review.coreboot.org/c/flashrom/+/33543

to review the following change.


Change subject: layout: Introduce flashrom_layout_empty()
......................................................................

layout: Introduce flashrom_layout_empty()

It initializes an empty layout. Currently the maximum number of entries
has to be specified, which will vanisch once we use dynamic allocation.

Change-Id: I2ae7246493ff592e631cce924777925c7825e398
Signed-off-by: Nico Huber <nico.h@gmx.de>
---
M cli_classic.c
M flash.h
M flashrom.c
M ich_descriptors.c
M ich_descriptors.h
M layout.c
M libflashrom.c
M libflashrom.h
8 files changed, 54 insertions(+), 37 deletions(-)



  git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/33543/1

diff --git a/cli_classic.c b/cli_classic.c
index 2e07612..a98be47 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -673,8 +673,10 @@
 out_shutdown:
 	programmer_shutdown();
 out:
-	for (i = 0; i < chipcount; i++)
+	for (i = 0; i < chipcount; i++) {
+		flashrom_layout_release(flashes[i].default_layout);
 		free(flashes[i].chip);
+	}
 
 	layout_cleanup(&include_args);
 	free(filename);
diff --git a/flash.h b/flash.h
index 5bbfa0a..84de7a7 100644
--- a/flash.h
+++ b/flash.h
@@ -252,7 +252,7 @@
 	chipaddr virtual_registers;
 	struct registered_master *mst;
 	const struct flashrom_layout *layout;
-	struct single_layout fallback_layout;
+	struct flashrom_layout *default_layout;
 	struct {
 		bool force;
 		bool force_boardmismatch;
diff --git a/flashrom.c b/flashrom.c
index e355731..3436a59 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1331,15 +1331,12 @@
 	if (!flash->chip)
 		return -1;
 
-	/* Fill fallback layout covering the whole chip. */
-	struct single_layout *const fallback = &flash->fallback_layout;
-	fallback->base.entries		= &fallback->entry;
-	fallback->base.max_entries	= 1;
-	fallback->base.num_entries	= 0;
-	if (flashrom_layout_add_region(&fallback->base,
+	/* Fill default layout covering the whole chip. */
+	if (flashrom_layout_empty(&flash->default_layout, 1) ||
+	    flashrom_layout_add_region(flash->default_layout,
 			0, flash->chip->total_size * 1024 - 1, "complete flash") ||
-	    flashrom_layout_include_region(&fallback->base, "complete flash"))
-		return -1;
+	    flashrom_layout_include_region(flash->default_layout, "complete flash"))
+	        return -1;
 
 	tmp = flashbuses_to_text(flash->chip->bustype);
 	msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) ", force ? "Assuming" : "Found",
diff --git a/ich_descriptors.c b/ich_descriptors.c
index b2c5d4c..8383801 100644
--- a/ich_descriptors.c
+++ b/ich_descriptors.c
@@ -1156,7 +1156,9 @@
  *	   1 if the descriptor couldn't be parsed,
  *	   2 when out of memory.
  */
-int layout_from_ich_descriptors(struct ich_layout *const layout, const void *const dump, const size_t len)
+int layout_from_ich_descriptors(
+		struct flashrom_layout **const layout,
+		const void *const dump, const size_t len)
 {
 	static const char *const regions[] = {
 		"fd", "bios", "me", "gbe", "pd", "reg5", "bios2", "reg7", "ec", "reg9", "ie",
@@ -1168,10 +1170,8 @@
 	if (read_ich_descriptors_from_dump(dump, len, &cs, &desc))
 		return 1;
 
-	memset(layout, 0x00, sizeof(*layout));
-	layout->base.entries = layout->entries;
-	layout->base.max_entries = ARRAY_SIZE(layout->entries);
-	layout->base.num_entries = 0;
+	if (flashrom_layout_empty(layout, ARRAY_SIZE(regions)))
+		return 2;
 
 	ssize_t i;
 	for (i = 0; i < min(ich_number_of_regions(cs, &desc.content), ARRAY_SIZE(regions)); ++i) {
@@ -1179,8 +1179,11 @@
 		const chipoff_t limit = ICH_FREG_LIMIT(desc.region.FLREGs[i]);
 		if (limit <= base)
 			continue;
-		if (flashrom_layout_add_region(&layout->base, base, limit, regions[i]))
+		if (flashrom_layout_add_region(*layout, base, limit, regions[i])) {
+			flashrom_layout_release(*layout);
+			*layout = NULL;
 			return 2;
+		}
 	}
 	return 0;
 }
diff --git a/ich_descriptors.h b/ich_descriptors.h
index 4225286..498af21 100644
--- a/ich_descriptors.h
+++ b/ich_descriptors.h
@@ -562,11 +562,6 @@
 	struct ich_desc_upper_map upper;
 };
 
-struct ich_layout {
-	struct flashrom_layout base;
-	struct romentry entries[MAX_NUM_FLREGS];
-};
-
 ssize_t ich_number_of_regions(enum ich_chipset cs, const struct ich_desc_content *content);
 ssize_t ich_number_of_masters(enum ich_chipset cs, const struct ich_desc_content *content);
 
@@ -585,6 +580,6 @@
 int read_ich_descriptors_via_fdo(enum ich_chipset cs, void *spibar, struct ich_descriptors *desc);
 int getFCBA_component_density(enum ich_chipset cs, const struct ich_descriptors *desc, uint8_t idx);
 
-int layout_from_ich_descriptors(struct ich_layout *, const void *dump, size_t len);
+int layout_from_ich_descriptors(struct flashrom_layout **, const void *dump, size_t len);
 
 #endif /* __ICH_DESCRIPTORS_H__ */
diff --git a/layout.c b/layout.c
index 0978a67..8198f17 100644
--- a/layout.c
+++ b/layout.c
@@ -37,7 +37,7 @@
 	if (flashctx->layout && flashctx->layout->num_entries)
 		return flashctx->layout;
 	else
-		return &flashctx->fallback_layout.base;
+		return flashctx->default_layout;
 }
 
 #ifndef __LIBPAYLOAD__
@@ -270,6 +270,29 @@
  */
 
 /**
+ * @brief Create a new, empty layout.
+ *
+ * @param layout Pointer to returned layout reference.
+ * @param count  Number of layout entries to allocate.
+ *
+ * @return 0 on success,
+ *         1 if out of memory.
+ */
+int flashrom_layout_empty(struct flashrom_layout **const layout, const unsigned int count)
+{
+	*layout = malloc(sizeof(**layout) + count * sizeof(struct romentry));
+	if (!*layout) {
+		msg_gerr("Error creating layout: %s\n", strerror(errno));
+		return 1;
+	}
+
+	(*layout)->entries	= (void *)((char *)*layout + sizeof(**layout));
+	(*layout)->max_entries	= count;
+	(*layout)->num_entries	= 0;
+	return 0;
+}
+
+/**
  * @brief Add another region to an existing layout.
  *
  * @param layout The existing layout.
diff --git a/libflashrom.c b/libflashrom.c
index aa43e65..d891944 100644
--- a/libflashrom.c
+++ b/libflashrom.c
@@ -205,13 +205,14 @@
 			ret = 0;
 			/* We found one chip, now check that there is no second match. */
 			if (probe_flash(®istered_masters[i], flash_idx + 1, &second_flashctx, 0) != -1) {
+				flashrom_layout_release(second_flashctx.default_layout);
 				ret = 3;
 				break;
 			}
 		}
 	}
 	if (ret) {
-		free(*flashctx);
+		flashrom_flash_release(*flashctx);
 		*flashctx = NULL;
 	}
 	return ret;
@@ -235,6 +236,7 @@
  */
 void flashrom_flash_release(struct flashrom_flashctx *const flashctx)
 {
+	flashrom_layout_release(flashctx->default_layout);
 	free(flashctx);
 }
 
@@ -310,16 +312,10 @@
 #ifndef __FLASHROM_LITTLE_ENDIAN__
 	return 6;
 #else
-	struct ich_layout dump_layout;
+	struct flashrom_layout *dump_layout, *chip_layout;
 	int ret = 1;
 
 	void *const desc = malloc(0x1000);
-	struct ich_layout *const chip_layout = malloc(sizeof(*chip_layout));
-	if (!desc || !chip_layout) {
-		msg_gerr("Out of memory!\n");
-		goto _free_ret;
-	}
-
 	if (prepare_flash_access(flashctx, true, false, false, false))
 		goto _free_ret;
 
@@ -332,7 +328,7 @@
 	}
 	msg_cinfo("done.\n");
 
-	if (layout_from_ich_descriptors(chip_layout, desc, 0x1000)) {
+	if (layout_from_ich_descriptors(&chip_layout, desc, 0x1000)) {
 		msg_cerr("Couldn't parse the descriptor!\n");
 		ret = 3;
 		goto _finalize_ret;
@@ -345,11 +341,11 @@
 			goto _finalize_ret;
 		}
 
-		const struct romentry *chip_entry = layout_next(&chip_layout->base, NULL);
-		const struct romentry *dump_entry = layout_next(&dump_layout.base, NULL);
+		const struct romentry *chip_entry = layout_next(chip_layout, NULL);
+		const struct romentry *dump_entry = layout_next(dump_layout, NULL);
 		while (chip_entry && dump_entry && !memcmp(chip_entry, dump_entry, sizeof(*chip_entry))) {
-			chip_entry = layout_next(&chip_layout->base, chip_entry);
-			dump_entry = layout_next(&dump_layout.base, dump_entry);
+			chip_entry = layout_next(chip_layout, chip_entry);
+			dump_entry = layout_next(dump_layout, dump_entry);
 		}
 		if (chip_entry || dump_entry) {
 			msg_cerr("Descriptors don't match!\n");
@@ -365,7 +361,7 @@
 	finalize_flash_access(flashctx);
 _free_ret:
 	if (ret)
-		free(chip_layout);
+		flashrom_layout_release(chip_layout);
 	free(desc);
 	return ret;
 #endif
diff --git a/libflashrom.h b/libflashrom.h
index 2b84bd4..90f3e2c 100644
--- a/libflashrom.h
+++ b/libflashrom.h
@@ -62,6 +62,7 @@
 int flashrom_image_verify(struct flashrom_flashctx *, const void *buffer, size_t buffer_len);
 
 struct flashrom_layout;
+int flashrom_layout_empty(struct flashrom_layout **layout, unsigned int count);
 int flashrom_layout_read_from_ifd(struct flashrom_layout **, struct flashrom_flashctx *, const void *dump, size_t len);
 int flashrom_layout_read_fmap_from_rom(struct flashrom_layout **,
 		struct flashrom_flashctx *, off_t offset, size_t length);

-- 
To view, visit https://review.coreboot.org/c/flashrom/+/33543
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2ae7246493ff592e631cce924777925c7825e398
Gerrit-Change-Number: 33543
Gerrit-PatchSet: 1
Gerrit-Owner: Nico Huber <nico.h@gmx.de>
Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com>
Gerrit-Reviewer: Thomas Heijligen <src@posteo.de>
Gerrit-MessageType: newchange

    

Change in ...flashrom[master]: layout: Introduce flashrom_layout_empty()

Nico Huber (Code Review)

Nico Huber (Code Review)

Angel Pons (Code Review)

Nico Huber (Code Review)

Nico Huber (Code Review)

Peter Marheine (Code Review)

Peter Marheine (Code Review)

Nico Huber (Code Review)

Nico Huber (Code Review)

Nico Huber (Code Review)

Anastasia Klimchuk (Code Review)

Peter Marheine (Code Review)

Edward O'Callaghan (Code Review)

Nico Huber (Code Review)

Nico Huber (Code Review)

Peter Marheine (Code Review)

Nico Huber (Code Review)

Peter Marheine (Code Review)

Angel Pons (Code Review)

Angel Pons (Code Review)

Peter Marheine (Code Review)

Anastasia Klimchuk (Code Review)

Nico Huber (Code Review)

Nico Huber (Code Review)

Edward O'Callaghan (Code Review)

Angel Pons (Code Review)

Nico Huber (Code Review)

tags

participants (5)