Change in flashrom[master]: layout: Add -i <region>[:<file>] support.

[David Hendricks (Code Review)]

Hello Carl-Daniel Hailfinger,

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

https://review.coreboot.org/23021

to review the following change.

Change subject: layout: Add -i <region>[:<file>] support.
......................................................................

layout: Add -i <region>[:<file>] support.

Add an optional sub-parameter to the -i parameter to allow building the
image to be written from multiple files. This will also allow regions to
be read from flash and written to separate image files.

This is a rebase of a patch that was ported from chromiumos. A lot of
things have changed, but the idea is the same.

Original patch by Louis Yung-Chieh Lo :
Summary: Support -i partition:file feature for both read and write.
Commit: 9c7525f
Review URL: http://codereview.chromium.org/6611015

Ported version by Stefan Tauner 
and Carl-Daniel Hailfinger :
Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support.
Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html

Change-Id: Ic5465659605d8431d931053967b40290195cfd99
Signed-off-by: David Hendricks 
Signed-off-by: Stefan Tauner 
Signed-off-by: Carl-Daniel Hailfinger 
---
M cli_classic.c
M dummyflasher.c
M flash.h
M flashrom.8.tmpl
M flashrom.c
M layout.c
M layout.h
7 files changed, 181 insertions(+), 38 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/1

diff --git a/cli_classic.c b/cli_classic.c
index 441fc91..c1481ec 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -58,7 +58,8 @@
           " -N | --noverify-all                verify included regions only (cf. -i)\n"
           " -l | --layout <layoutfile>         read ROM layout from <layoutfile>\n"
           "      --ifd                         read layout from an Intel Firmware Descriptor\n"
-	       " -i | --image <name>                only flash image <name> from flash layout\n"
+	       " -i | --include <region>[<:file>]   only flash image <region> from layout\n"
+	       "                                    (optionally with data from <file>)\n"
           " -o | --output <logfile>            log output to <logfile>\n"
           " -L | --list-supported              print supported devices\n"
 #if CONFIG_PRINT_WIKI == 1
diff --git a/dummyflasher.c b/dummyflasher.c
index f171128..1fa67d8 100644
--- a/dummyflasher.c
+++ b/dummyflasher.c
@@ -139,8 +139,10 @@
 #if EMULATE_CHIP
    if (emu_chip != EMULATE_NONE) {
    	if (emu_persistent_image) {
+			struct flashctx flash = { 0 };
+
    		msg_pdbg("Writing %s\n", emu_persistent_image);
-			write_buf_to_file(flashchip_contents, emu_chip_size, emu_persistent_image);
+			write_buf_to_file(&flash, flashchip_contents, emu_persistent_image);
    		free(emu_persistent_image);
    		emu_persistent_image = NULL;
    	}
@@ -382,7 +384,7 @@
    		msg_pdbg("matches.\n");
    		msg_pdbg("Reading %s\n", emu_persistent_image);
    		read_buf_from_file(flashchip_contents, emu_chip_size,
-					   emu_persistent_image);
+					   emu_persistent_image, NULL);
    	} else {
    		msg_pdbg("doesn't match.\n");
    	}
diff --git a/flash.h b/flash.h
index 62f2e34..b8344fc 100644
--- a/flash.h
+++ b/flash.h
@@ -296,8 +296,8 @@
 void print_banner(void);
 void list_programmers_linebreak(int startcol, int cols, int paren);
 int selfcheck(void);
-int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename);
-int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename);
+int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename, const char *size_msg);
+int write_buf_to_file(struct flashctx *const flash, const unsigned char *buf, const char *filename);
 int prepare_flash_access(struct flashctx *, bool read_it, bool write_it, bool erase_it, bool verify_it);
 void finalize_flash_access(struct flashctx *);
 int do_read(struct flashctx *, const char *filename);
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index 143d76c..6a05e8a 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -48,7 +48,7 @@
 \fB-p\fR <programmername>[:<parameters>]
                [\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\fB-v\fR <file>] \
 [\fB-c\fR <chipname>]
-               [(\fB-l\fR <file>|\fB--ifd\fR) [\fB-i\fR <image>]] \
+               [(\fB-l\fR <file>|\fB--ifd\fR) [\fB-i\fR <image>[:<file>]]] \
 [\fB-n\fR] [\fB-N\fR] [\fB-f\fR]]
          [\fB-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>]
 .SH DESCRIPTION
@@ -211,10 +211,45 @@
   gbe   gigabit ethernet firmware
   pd    platform specific data
 .TP
-.B "-i, --image <imagename>"
-Only flash region/image
-.B <imagename>
-from flash layout.
+.B "-i, --include <region>[:<file>]"
+Read or write only
+.B <region>
+to or from ROM.
+The
+.B "-i"
+option may be used multiple times if the user wishes to read or write
+multiple regions using a single command.
+.sp
+The user may optionally specify a corresponding
+.B <file>
+for any region they wish to read or write. A read operation will read the
+corresponding regions from ROM and write individual files for each one. A write
+write option will read file(s) and write to the corresponding region(s) in ROM.
+.sp
+For write operations, files specified using
+.B "-i"
+take precedence over content from the argument to
+.B "-w."
+.sp
+Examples:
+.sp
+  To read regions named
+.BR "foo " and " bar"
+in layout file
+.B <layout>
+into region-sized files
+.BR "foo.bin " and " bar.bin" ", run:
+.sp
+.B "    flashrom -p prog -l <layout> -i foo:foo.bin -i bar:bar.bin -r rom.bin
+.sp
+  To write files
+.BR "foo.bin " and " bar.bin"
+into regions named
+.BR "foo " and " bar" " in layout file
+.BR <layout>
+to the ROM, run:
+.sp
+.B "    flashrom -p prog -l <layout> -i foo:foo.bin -i bar:bar.bin -w rom.bin
 .TP
 .B "-L, --list-supported"
 List the flash chips, chipsets, mainboards, and external programmers
diff --git a/flashrom.c b/flashrom.c
index 12d7390..4f50c54 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1315,8 +1315,7 @@
    return chip - flashchips;
 }

-int read_buf_from_file(unsigned char *buf, unsigned long size,
-		       const char *filename)
+int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename, const char *size_msg)
 {
 #ifdef __LIBPAYLOAD__
    msg_gerr("Error: No file I/O support in libpayload\n");
@@ -1337,8 +1336,10 @@
    	goto out;
    }
    if (image_stat.st_size != size) {
-		msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n",
-			 (intmax_t)image_stat.st_size, size);
+		if (size_msg == NULL)
+			size_msg = "the expected size";
+		msg_gerr("Error: Image size (%jd B) doesn't match %s (%lu B)!\n",
+			 (intmax_t)image_stat.st_size, size_msg, size);
    	ret = 1;
    	goto out;
    }
@@ -1355,12 +1356,21 @@
 #endif
 }

-int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename)
+/**
+ * @brief Writes included layout regions into buffer(s)
+ *
+ * If partial read to file is to be done (-i <region>[:<regionfile>] syntax
+ * used) then this will write to corresponding region-sized files. If an
+ * argument to -r/-w/-v is specified then it will write a chip-sized file. Both
+ * partial read/writes and full read/write may be done at once.
+ *
+ * @param buf      Buffer with data to write
+ * @param size     Size of buffer
+ * @param filename Filename corresponding to optional argument to -r/-w/-v
+ * @return 0 on success
+ */
+static int do_write_buf_to_file(const unsigned char *buf, unsigned long size, const char *filename)
 {
-#ifdef __LIBPAYLOAD__
-	msg_gerr("Error: No file I/O support in libpayload\n");
-	return 1;
-#else
    FILE *image;
    int ret = 0;

@@ -1404,9 +1414,59 @@
    	ret = 1;
    }
    return ret;
+}
+
+/**
+ * @brief Writes content from buffer to one or more files.
+ *
+ * Writes content from supplied buffer to files. If a filename is specified for
+ * individual regions using the partial read syntax ('-i <region>[:<filename>]')
+ * then this will write files using data from the corresponding region in the
+ * supplied buffer. If a filename is specified for -r/-w/-e options then a chip-
+ * sized file will be created.
+ *
+ * @param flashctx Flash context to be used.
+ * @param buf      Chip-sized buffer to read data from
+ * @param filename Optional filename corresponding to -r/-w/-v argument
+ * @return 0 on success
+ */
+int write_buf_to_file(struct flashctx *const flash, const unsigned char *buf, const char *filename)
+{
+#ifdef __LIBPAYLOAD__
+	msg_gerr("Error: No file I/O support in libpayload\n");
+	return 1;
+#else
+	int ret = 0;
+	const struct flashrom_layout *const layout = get_layout(flash);
+
+	size_t i;
+	for (i = 0; i < layout->num_entries; i++) {
+		if (!layout->entries[i].included)
+			continue;
+		if (!layout->entries[i].file)
+			continue;
+
+		const chipoff_t region_start	= layout->entries[i].start;
+		const chipsize_t region_len	= layout->entries[i].end - layout->entries[i].start + 1;
+		const char *region_file		= layout->entries[i].file;
+
+		ret = do_write_buf_to_file(buf + region_start, region_len, region_file);
+		if (ret)
+			goto out;
+	}
+
+	if (filename) {
+		ret = do_write_buf_to_file(buf, flash->chip->total_size * 1024, filename);
+		if (ret)
+			goto out;
+	}
+
+out:
+	return ret;
 #endif
 }

+
 static int read_by_layout(struct flashctx *, uint8_t *);
 int read_flash_to_file(struct flashctx *flash, const char *filename)
 {
@@ -1431,7 +1491,7 @@
    	goto out_free;
    }

-	ret = write_buf_to_file(buf, size, filename);
+	ret = write_buf_to_file(flash, buf, filename);
 out_free:
    free(buf);
    msg_cinfo("%s.\n", ret ? "FAILED" : "done");
@@ -2538,7 +2598,7 @@
    	goto _free_ret;
    }

-	if (read_buf_from_file(newcontents, flash_size, filename))
+	if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
    	goto _free_ret;

ret = flashrom_image_write(flash, newcontents, flash_size);
@@ -2559,7 +2619,7 @@
    	goto _free_ret;
    }

-	if (read_buf_from_file(newcontents, flash_size, filename))
+	if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
    	goto _free_ret;

ret = flashrom_image_verify(flash, newcontents, flash_size);
diff --git a/layout.c b/layout.c
index 7ce7c57..4ae4a69 100644
--- a/layout.c
+++ b/layout.c
@@ -58,7 +58,7 @@
    romlayout = fopen(name, "r");

if (!romlayout) {
-		msg_gerr("ERROR: Could not open ROM layout (%s).\n",
+		msg_gerr("ERROR: Could not open layout file (%s).\n",
    		name);
    	return -1;
    }
@@ -67,8 +67,7 @@
    	char *tstr1, *tstr2;

if (layout.num_entries >= MAX_ROMLAYOUT) {
-			msg_gerr("Maximum number of ROM images (%i) in layout "
-				 "file reached.\n", MAX_ROMLAYOUT);
+			msg_gerr("Maximum number of entries (%i) in layout file reached.\n", MAX_ROMLAYOUT);
    		(void)fclose(romlayout);
    		return 1;
    	}
@@ -90,6 +89,7 @@
    	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.entries[layout.num_entries].file = NULL;
    	layout.num_entries++;
    }

@@ -150,7 +150,6 @@
    msg_gspew("Looking for region "%s"... ", name);
    for (i = 0; i < l->num_entries; i++) {
    	if (!strcmp(l->entries[i].name, name)) {
-			l->entries[i].included = 1;
    		msg_gspew("found.\n");
    		return i;
    	}
@@ -170,28 +169,71 @@
    if (num_include_args == 0)
    	return 0;

-	/* User has specified an area, but no layout file is loaded. */
+	/* User has specified an include argument, but no layout is loaded. */
    if (l->num_entries == 0) {
-		msg_gerr("Region requested (with -i "%s"), "
-			 "but no layout data is available.\n",
+		msg_gerr("Region requested (with -i/--include "%s"),\n"
+			 "but no layout data is available. Use the -l/--layout parameter to specify\n"
+			 "a layout file.\n",
    		 include_args[0]);
    	return 1;
    }

for (i = 0; i < num_include_args; i++) {
-		if (find_romentry(l, include_args[i]) < 0) {
-			msg_gerr("Invalid region specified: "%s".\n",
-				 include_args[i]);
+		char *name = include_args[i];
+		char *file = strpbrk(name, ":"); /* -i <region>[:<file>] */
+		if (file != NULL) {
+			*file = '\0';
+			file++;
+		}
+
+		/* Quotes or whitespace are not allowed in region names. */
+		if (strlen(name) == 0 || strpbrk(name, "" \t\r\n")) {
+			msg_gerr("Invalid region name specified: "%s".\n", name);
    		return 1;
    	}
-		found++;
+
+		int idx = find_romentry(l, name);
+		if (idx < 0) {
+			msg_gerr("Nonexisting region name specified: "%s".\n", name);
+			return 1;
+		}
+		if (l->entries[idx].included == 0) {
+			l->entries[idx].included = 1;
+			found++;
+		} else {
+			msg_gerr("Duplicate region name specified: "%s".\n", name);
+			return 1;
+		}
+
+		if (file) {
+			if (strlen(file) == 0) {
+				msg_gerr("Empty region file name specified for region "%s".\n", name);
+				return 1;
+			}
+#ifdef __LIBPAYLOAD__
+			msg_gerr("Error: No file I/O support in libpayload\n");
+			return 1;
+#else
+			file = strdup(file);
+			if (file == NULL) {
+				msg_gerr("Out of memory!\n");
+				return 1;
+			}
+			l->entries[idx].file = file;
+#endif
+		}
    }

-	msg_ginfo("Using region%s: "%s"", num_include_args > 1 ? "s" : "",
-		  include_args[0]);
-	for (i = 1; i < num_include_args; i++)
-		msg_ginfo(", "%s"", include_args[i]);
-	msg_ginfo(".\n");
+	msg_gdbg("Using %i region%s: ", found, found > 1 ? "s" : "");
+	for (i = 0; i < l->num_entries; i++)
+		if (l->entries[i].included) {
+			msg_gdbg(""%s"", l->entries[i].name);
+			if (l->entries[i].file)
+				msg_gdbg(":"%s"", l->entries[i].file);
+			if (--found)
+				msg_gdbg(", ");
+	}
+	msg_gdbg(".\n");
    return 0;
 }

@@ -205,6 +247,8 @@
    num_include_args = 0;

for (i = 0; i < layout.num_entries; i++) {
+		free(layout.entries[i].file);
+		layout.entries[i].file = NULL;
    	layout.entries[i].included = 0;
    }
    layout.num_entries = 0;
diff --git a/layout.h b/layout.h
index fd1049d..d1c793f 100644
--- a/layout.h
+++ b/layout.h
@@ -43,6 +43,7 @@
    chipoff_t end;
    bool included;
    char name[256];
+	char *file;
 };

struct flashrom_layout {