Change in flashrom[staging]: Give layouts their own type

[David Hendricks (Code Review)]

Hello Nico Huber,

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

https://review.coreboot.org/19188

to review the following change.

Change subject: Give layouts their own type
......................................................................

Give layouts their own type

Introduce `struct fl_layout` and refactor layout.c a little, so we can
reuse the layout from there and have other sources of layouts beside it.

I didn't want to clutter up flash.h any more. So things went into a new
layout.h.

Change-Id: Ic728fc9f64958f62a2e2f321f4ec440e8658c808
Signed-off-by: Nico Huber 
---
M flash.h
M layout.c
A layout.h
3 files changed, 99 insertions(+), 50 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/88/19188/1

diff --git a/flash.h b/flash.h
index da049d1..bf381cf 100644
--- a/flash.h
+++ b/flash.h
@@ -37,6 +37,8 @@
 #undef max
 #endif

+#include "layout.h"
+
 #define ERROR_PTR ((void*)-1)

/* Error codes */
@@ -46,14 +48,6 @@
 /* TODO: check using code for correct usage of types */
 typedef uintptr_t chipaddr;
 #define PRIxPTR_WIDTH ((int)(sizeof(uintptr_t)*2))
-
-/* Types and macros regarding the maximum flash space size supported by generic code. */
-typedef uint32_t chipoff_t; /* Able to store any addressable offset within a supported flash memory. */
-typedef uint32_t chipsize_t; /* Able to store the number of bytes of any supported flash memory. */
-#define FL_MAX_CHIPOFF_BITS (24)
-#define FL_MAX_CHIPOFF ((chipoff_t)(1ULL<<FL_MAX_CHIPOFF_BITS)-1)
-#define PRIxCHIPOFF "06"PRIx32
-#define PRIuCHIPSIZE PRIu32

int register_shutdown(int (*function) (void *data), void *data);
 int shutdown_free(void *data);
diff --git a/layout.c b/layout.c
index f71eeaa..9eadb22 100644
--- a/layout.c
+++ b/layout.c
@@ -25,24 +25,20 @@
 #include <limits.h>
 #include "flash.h"
 #include "programmer.h"
+#include "layout.h"

-#define MAX_ROMLAYOUT	32
-
-typedef struct {
-	chipoff_t start;
-	chipoff_t end;
-	unsigned int included;
-	char name[256];
-} romentry_t;
-
-/* rom_entries store the entries specified in a layout file and associated run-time data */
-static romentry_t rom_entries[MAX_ROMLAYOUT];
-static int num_rom_entries = 0; /* the number of successfully parsed rom_entries */
+struct fl_romentry entries[MAX_ROMLAYOUT];
+static struct fl_layout layout = { entries, 0 };

/* include_args holds the arguments specified at the command line with -i. They must be processed at some point
- * so that desired regions are marked as "included" in the rom_entries list. */
+ * so that desired regions are marked as "included" in the layout. */
 static char *include_args[MAX_ROMLAYOUT];
 static int num_include_args = 0; /* the number of valid include_args. */
+
+const struct fl_layout *get_global_layout(void)
+{
+	return &layout;
+}

#ifndef __LIBPAYLOAD__
 int read_romlayout(const char *name)
@@ -62,13 +58,13 @@
    while (!feof(romlayout)) {
    	char *tstr1, *tstr2;

-		if (num_rom_entries >= MAX_ROMLAYOUT) {
+		if (layout.num_entries >= MAX_ROMLAYOUT) {
    		msg_gerr("Maximum number of ROM images (%i) in layout "
    			 "file reached.\n", MAX_ROMLAYOUT);
    		(void)fclose(romlayout);
    		return 1;
    	}
-		if (2 != fscanf(romlayout, "%255s %255s\n", tempstr, rom_entries[num_rom_entries].name))
+		if (2 != fscanf(romlayout, "%255s %255s\n", tempstr, layout.entries[layout.num_entries].name))
    		continue;
 #if 0
    	// fscanf does not like arbitrary comments like that :( later
@@ -83,16 +79,16 @@
    		(void)fclose(romlayout);
    		return 1;
    	}
-		rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16);
-		rom_entries[num_rom_entries].end = strtol(tstr2, (char **)NULL, 16);
-		rom_entries[num_rom_entries].included = 0;
-		num_rom_entries++;
+		layout.entries[layout.num_entries].start = strtol(tstr1, (char **)NULL, 16);
+		layout.entries[layout.num_entries].end = strtol(tstr2, (char **)NULL, 16);
+		layout.entries[layout.num_entries].included = 0;
+		layout.num_entries++;
    }

-	for (i = 0; i < num_rom_entries; i++) {
+	for (i = 0; i < layout.num_entries; i++) {
    	msg_gdbg("romlayout %08x - %08x named %s\n",
-			     rom_entries[i].start,
-			     rom_entries[i].end, rom_entries[i].name);
+			     layout.entries[i].start,
+			     layout.entries[i].end, layout.entries[i].name);
    }

(void)fclose(romlayout);
@@ -140,13 +136,13 @@
 {
    int i;

-	if (num_rom_entries == 0)
+	if (layout.num_entries == 0)
    	return -1;

msg_gspew("Looking for region "%s"... ", name);
-	for (i = 0; i < num_rom_entries; i++) {
-		if (!strcmp(rom_entries[i].name, name)) {
-			rom_entries[i].included = 1;
+	for (i = 0; i < layout.num_entries; i++) {
+		if (!strcmp(layout.entries[i].name, name)) {
+			layout.entries[i].included = 1;
    		msg_gspew("found.\n");
    		return i;
    	}
@@ -167,7 +163,7 @@
    	return 0;

/* User has specified an area, but no layout file is loaded. */
-	if (num_rom_entries == 0) {
+	if (layout.num_entries == 0) {
    	msg_gerr("Region requested (with -i "%s"), "
    		 "but no layout data is available.\n",
    		 include_args[0]);
@@ -200,22 +196,22 @@
    }
    num_include_args = 0;

-	for (i = 0; i < num_rom_entries; i++) {
-		rom_entries[i].included = 0;
+	for (i = 0; i < layout.num_entries; i++) {
+		layout.entries[i].included = 0;
    }
-	num_rom_entries = 0;
+	layout.num_entries = 0;
 }

-romentry_t *get_next_included_romentry(unsigned int start)
+struct fl_romentry *get_next_included_romentry(unsigned int start)
 {
    int i;
    unsigned int best_start = UINT_MAX;
-	romentry_t *best_entry = NULL;
-	romentry_t *cur;
+	struct fl_romentry *best_entry = NULL;
+	struct fl_romentry *cur;

/* First come, first serve for overlapping regions. */
-	for (i = 0; i < num_rom_entries; i++) {
-		cur = &rom_entries[i];
+	for (i = 0; i < layout.num_entries; i++) {
+		cur = &layout.entries[i];
    	if (!cur->included)
    		continue;
    	/* Already past the current entry? */
@@ -240,16 +236,16 @@
    int ret = 0;

int i;
-	for (i = 0; i < num_rom_entries; i++) {
-		if (rom_entries[i].start >= total_size || rom_entries[i].end >= total_size) {
+	for (i = 0; i < layout.num_entries; i++) {
+		if (layout.entries[i].start >= total_size || layout.entries[i].end >= total_size) {
    		msg_gwarn("Warning: Address range of region "%s" exceeds the current chip's "
-				  "address space.\n", rom_entries[i].name);
-			if (rom_entries[i].included)
+				  "address space.\n", layout.entries[i].name);
+			if (layout.entries[i].included)
    			ret = 1;
    	}
-		if (rom_entries[i].start > rom_entries[i].end) {
+		if (layout.entries[i].start > layout.entries[i].end) {
    		msg_gerr("Error: Size of the address range of region "%s" is not positive.\n",
-				  rom_entries[i].name);
+				  layout.entries[i].name);
    		ret = 1;
    	}
    }
@@ -281,7 +277,7 @@
 int build_new_image(struct flashctx *flash, bool oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents)
 {
    unsigned int start = 0;
-	romentry_t *entry;
+	struct fl_romentry *entry;
    unsigned int size = flash->chip->total_size * 1024;

/* If no regions were specified for inclusion, assume
diff --git a/layout.h b/layout.h
new file mode 100644
index 0000000..3aebe8a
--- /dev/null
+++ b/layout.h
@@ -0,0 +1,59 @@
+/*
+ * This file is part of the flashrom project.
+ *
+ * Copyright (C) 2005-2008 coresystems GmbH
+ * (Written by Stefan Reinauer  for coresystems GmbH)
+ * Copyright (C) 2011-2013 Stefan Tauner
+ * Copyright (C) 2016 secunet Security Networks AG
+ * (Written by Nico Huber  for secunet)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+#ifndef __LAYOUT_H__
+#define __LAYOUT_H__ 1
+
+/* Types and macros regarding the maximum flash space size supported by generic code. */
+typedef uint32_t chipoff_t; /* Able to store any addressable offset within a supported flash memory. */
+typedef uint32_t chipsize_t; /* Able to store the number of bytes of any supported flash memory. */
+#define FL_MAX_CHIPOFF_BITS (24)
+#define FL_MAX_CHIPOFF ((chipoff_t)(1ULL<<FL_MAX_CHIPOFF_BITS)-1)
+#define PRIxCHIPOFF "06"PRIx32
+#define PRIuCHIPSIZE PRIu32
+
+#define MAX_ROMLAYOUT	32
+
+struct fl_romentry {
+	chipoff_t start;
+	chipoff_t end;
+	bool included;
+	char name[256];
+};
+
+struct fl_layout {
+	/* entries store the entries specified in a layout file and associated run-time data */
+	struct fl_romentry *entries;
+	/* the number of successfully parsed entries */
+	int num_entries;
+};
+
+struct fl_layout_single {
+	struct fl_layout base;
+	struct fl_romentry entry;
+};
+
+const struct fl_layout *get_global_layout(void);
+
+#endif				/* !__LAYOUT_H__ */