Re: [flashrom] [PATCH 2/6] layout: Add -i <region>[:<file>] support.

[Carl-Daniel Hailfinger]

Am 08.10.2013 02:34 schrieb Carl-Daniel Hailfinger:
> Am 24.09.2013 01:14 schrieb Stefan Tauner:
> > 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 in a later patch.
> > Existing function read_buf_from_file() is refined and reused to read the file.
> >
> > based on chromiumos'
> > d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
> > Signed-off-by: David Hendricks 
> > Signed-off-by: Stefan Tauner
>
> Counter-proposal, slightly edited patch, comments below are addressed.
> > diff --git a/cli_classic.c b/cli_classic.c
> > index a0c2d64..ed6d6aa 100644
> > --- a/cli_classic.c
> > +++ b/cli_classic.c
> > @@ -40,8 +40,8 @@ static void cli_classic_usage(const char *name)
> >  #if CONFIG_PRINT_WIKI == 1
> >           "-z|"
> >  #endif
> >       "-p <programmername>[:<parameters>] [-c <chipname>]\n"
> >       "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...] [-n] [-f]]\n"
> >       "-p <programmername>[:<parameter>[,<parameter>]...] [-c <chipname>]\n"
> >       "[-E|(-r|-w|-v) <imagefile>] [-l <layoutfile> [-i <region>[:<regionfile>]...] [-n] [-f]]\n"
>
> 80 col violation. Unclosed [.
> >       "[-V[V[V]]] [-o <logfile>]\n\n", name);
> >
> > printf(" -h | --help                        print this help text\n"
> > diff --git a/layout.c b/layout.c
> > index 08cc776..7e2e904 100644
> > --- a/layout.c
> > +++ b/layout.c
> > @@ -165,27 +161,61 @@ int process_include_args(void)
> >    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 file is loaded. */
> > if (num_rom_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. To include one use the -l/--layout syntax).\n",
>
> "To include one [layout data] ..." sounds odd. I rewrote it slightly.
> > 	 include_args[0]);
> > return 1;
> >
> > }
> >
> > for (i = 0; i < num_include_args; i++) {
> > if (find_romentry(include_args[i]) < 0) {
> > 	msg_gerr("Invalid region specified: \"%s\".\n",
> > 		 include_args[i]);
> > char *file;
> > char *name = include_args[i];
> > int ret = unquote_string(&name, &file, ":"); /* -i <region>[:<file>] */
>
> I killed the unquote_string requirement here. Whitespace and colons have
> been outlawed in region names.
> > if (ret != 0) {
> > 	msg_gerr("Invalid include argument specified: \"%s\".\n", name);
> > return 1;
> > }
> > int idx = find_romentry(name);
> > if (idx < 0) {
> > 	msg_gerr("Invalid region name specified: \"%s\".\n", name);
> > 	return 1;
> > }
> > rom_entries[idx].included = 1;
> > found++;
> > if (file[0] != '\0') {
> > 	/* The remaining characters are interpreted as possible quoted filename.  */
> > 	ret = unquote_string(&file, NULL, NULL);
>
> A file name on the command line should never require quote parsing in
> the application. Same for any other flashrom UI.
> > 	if (ret != 0) {
> > 		msg_gerr("Invalid region file name specified: \"%s\".\n", file);
> > 		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;
> > 	}
> > 	rom_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("Using region%s: ", num_rom_entries > 1 ? "s" : "");
>
> Should be found, not num_rom_entries.
> > bool first = true;
> > for (i = 0; i < num_rom_entries; i++)
> > if (rom_entries[i].included) {
> > 	if (first)
> > 		first = false;
> > 	else
> > 		msg_ginfo(", ");
> > 	msg_ginfo("\"%s\"", rom_entries[i].name);
> > }
> > msg_ginfo(".\n");
> > return 0;
> >
> > }
>
> There was another bug: register_include_arg() tried to detect duplicate
> include region arguments. It could only detect identical include
> strings, but not stuff like this:
> -i region1 -i region1:region1file.bin
> The duplicate --include detection has been moved to process_include_args().
> Patch 1/6 is no longer a requirement for 2/6 and can thus be committed
> after 2/6.
>
> 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 in a later patch.
> Existing function read_buf_from_file() is refined and reused to read the
> file.
> Document criteria for valid region names.
> Based on chromiumos' d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
>
> Signed-off-by: David Hendricks 
> Signed-off-by: Stefan Tauner 
> Signed-off-by: Carl-Daniel Hailfinger

Latest version.
Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-layout_read_region_from_file/flash.h
===================================================================
--- flashrom-layout_read_region_from_file/flash.h	(Revision 1757)
+++ flashrom-layout_read_region_from_file/flash.h	(Arbeitskopie)
@@ -261,7 +261,7 @@
 void list_programmers_linebreak(int startcol, int cols, int paren);
 int selfcheck(void);
 int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it);
-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);
 int write_buf_to_file(unsigned char *buf, unsigned long size, const char *filename);

enum test_state {
Index: flashrom-layout_read_region_from_file/dummyflasher.c
===================================================================
--- flashrom-layout_read_region_from_file/dummyflasher.c	(Revision 1757)
+++ flashrom-layout_read_region_from_file/dummyflasher.c	(Arbeitskopie)
@@ -389,7 +389,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");
    	}
Index: flashrom-layout_read_region_from_file/cli_classic.c
===================================================================
--- flashrom-layout_read_region_from_file/cli_classic.c	(Revision 1757)
+++ flashrom-layout_read_region_from_file/cli_classic.c	(Arbeitskopie)
@@ -34,15 +34,15 @@
 static void cli_classic_usage(const char *name)
 {
    printf("Please note that the command line interface for flashrom has changed between\n"
-	       "0.9.5 and 0.9.6 and will change again before flashrom 1.0.\n\n");
+	       "0.9.7 and 0.9.8 and will change again before flashrom 1.0.\n\n");

printf("Usage: %s [-h|-R|-L|"
 #if CONFIG_PRINT_WIKI == 1
           "-z|"
 #endif
-	       "-p <programmername>[:<parameters>] [-c <chipname>]\n"
-	       "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...] [-n] [-f]]\n"
-	       "[-V[V[V]]] [-o <logfile>]\n\n", name);
+	       "-p <programmername>[:<parameter>[,<parameter>]...]\n"
+	       " [-E|(-r|-w|-v) <imagefile>] [-l <layoutfile> [-i <region>[:<regionfile>]]...]\n"
+	       " [-c <chipname>] [-n] [-f]] [-V[V[V]]] [-o <logfile>]\n\n", name);

printf(" -h | --help                        print this help text\n"
           " -R | --version                     print version (release)\n"
@@ -54,8 +54,9 @@
           " -c | --chip <chipname>             probe only for specified flash chip\n"
           " -f | --force                       force specific operations (see man page)\n"
           " -n | --noverify                    don't auto-verify\n"
-	       " -l | --layout <layoutfile>         read ROM layout from <layoutfile>\n"
-	       " -i | --image <name>                only flash image <name> from flash layout\n"
+	       " -l | --layout <layoutfile>         read layout from <layoutfile>\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
Index: flashrom-layout_read_region_from_file/flashrom.8.tmpl
===================================================================
--- flashrom-layout_read_region_from_file/flashrom.8.tmpl	(Revision 1757)
+++ flashrom-layout_read_region_from_file/flashrom.8.tmpl	(Arbeitskopie)
@@ -3,10 +3,10 @@
 flashrom - detect, read, write, verify and erase flash chips
 .SH SYNOPSIS
 .B flashrom \fR[\fB-h\fR|\fB-R\fR|\fB-L\fR|\fB-z\fR|\
-\fB-p\fR <programmername>[:<parameters>]
-               [\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\fB-v\fR <file>] \
+\fB-p\fR <programmername>[:<parameter>[,<parameter>]...]
+               [\fB-E\fR|\fB-r\fR <imagefile>|\fB-w\fR <imagefile>|\fB-v\fR <imagefile>] \
 [\fB-c\fR <chipname>]
-               [\fB-l\fR <file> [\fB-i\fR <image>]] [\fB-n\fR] [\fB-f\fR]]
+               [\fB-l\fR <layoutfile> [\fB-i\fR <regionname>[:<regionfile>]]...] [\fB-n\fR] [\fB-f\fR]]
          [\fB-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>]
 .SH DESCRIPTION
 .B flashrom
@@ -22,8 +22,9 @@
 parallel flash, or SPI.
 .SH OPTIONS
 .B IMPORTANT:
-Please note that the command line interface for flashrom will change before
-flashrom 1.0. Do not use flashrom in scripts or other automated tools without
+Please note that the command line interface for flashrom has changed between
+flashrom 0.9.7 and 0.9.8 and will change again before flashrom 1.0.
+Do not use flashrom in scripts or other automated tools without
 checking that your flashrom version won't interpret options in a different way.
 .PP
 You can specify one of
@@ -38,14 +39,14 @@
 .B -p/--programmer
 option to be used (please see below).
 .TP
-.B "-r, --read <file>"
+.B "-r, --read <imagefile>"
 Read flash ROM contents and save them into the given
-.BR <file> .
+.BR <imagefile> .
 If the file already exists, it will be overwritten.
 .TP
-.B "-w, --write <file>"
+.B "-w, --write <imagefile>"
 Write
-.B <file>
+.B <imagefile>
 into flash ROM. This will first automatically
 .B erase
 the chip, then write to it.
@@ -65,14 +66,14 @@
 feel that the time for verification takes too long.
 .sp
 Typical usage is:
-.B "flashrom -p prog -n -w <file>"
+.B "flashrom -p prog -n -w <imagefile>"
 .sp
 This option is only useful in combination with
 .BR --write .
 .TP
-.B "-v, --verify <file>"
+.B "-v, --verify <imagefile>"
 Verify the flash ROM contents against the given
-.BR <file> .
+.BR <imagefile> .
 .TP
 .B "-E, --erase"
 Erase the flash ROM chip.
@@ -102,46 +103,6 @@
 .sp
 * Force write even if write is known bad.
 .TP
-.B "-l, --layout <file>"
-Read ROM layout from
-.BR <file> .
-.sp
-flashrom supports ROM layouts. This allows you to flash certain parts of
-the flash chip only. A ROM layout file contains multiple lines with the
-following syntax:
-.sp
-.B "  startaddr:endaddr imagename"
-.sp
-.BR "startaddr " "and " "endaddr "
-are hexadecimal addresses within the ROM file and do not refer to any
-physical address. Please note that using a 0x prefix for those hexadecimal
-numbers is not necessary, but you can't specify decimal/octal numbers.
-.BR "imagename " "is an arbitrary name for the region/image from"
-.BR " startaddr " "to " "endaddr " "(both addresses included)."
-.sp
-Example:
-.sp
-  00000000:00008fff gfxrom
-  00009000:0003ffff normal
-  00040000:0007ffff fallback
-.sp
-If you only want to update the image named
-.BR "normal " "in a ROM based on the layout above, run"
-.sp
-.B "  flashrom -p prog --layout rom.layout --image normal -w some.rom"
-.sp
-To update only the images named
-.BR "normal " "and " "fallback" ", run:"
-.sp
-.B "  flashrom -p prog -l rom.layout -i normal -i fallback -w some.rom"
-.sp
-Overlapping sections are not supported.
-.TP
-.B "-i, --image <imagename>"
-Only flash region/image
-.B <imagename>
-from flash layout.
-.TP
 .B "-L, --list-supported"
 List the flash chips, chipsets, mainboards, and external programmers
 (including PCI, USB, parallel port, and serial port based devices)
@@ -225,6 +186,53 @@
 in detail in the
 .B PROGRAMMER SPECIFIC INFO
 section. Support for some programmers can be disabled at compile time.
+.TP
+flashrom supports reading/writing/erasing/verifying flash chips in whole (default) or in part. To access only \
+parts of a chip one has to use layout files and respective arguments described below.
+.TP
+.B "-l, --layout <layoutfile>"
+Read layout entries from
+.BR <layoutfile> .
+.sp
+Each layout entry describes an address region of the flash chip and gives it a name (hereinafter referred to as
+a region). One entry per line is allowed with the following syntax:
+.sp
+.B "  startaddr:endaddr regionname"
+.sp
+.BR "startaddr " "and " "endaddr "
+are hexadecimal addresses within the ROM image representing the flash ROM contents and do not refer to any
+physical address. Please note that using a 0x prefix for those hexadecimal numbers is not necessary, but you
+can't specify decimal/octal numbers.
+.BR "regionname " "is the name for the region from " "startaddr " "to " "endaddr " "(both addresses included)."
+Spaces, tabs, newlines, colons, single or double quotes are not allowed in region names.
+.sp
+Example content of file rom.layout:
+.sp
+  0x00000000:0x00008fff gfx_rom
+  0x00009000:0x0003ffff normal
+  0x00040000:0x0007ffff fallback
+.sp
+.TP
+.B "-i, --include <name>[:<regionfile>]"
+Work on the flash region
+.B name
+instead of the full address space if a layout file is given and parsed correctly.
+Multiple such include parameters can be used to work on the union of different regions.
+.sp
+The optional
+.B regionfile
+parameter specifies the name of a file that is used to map the contents of that very region only.
+The optional file parameter causes the contents of this region to be replaced by the contents of the file
+specified here.
+.sp
+If you only want to update the regions named
+.BR "normal " "and " "gfx_rom " "in a ROM based on the layout mentioned above, run"
+.sp
+.B "  flashrom -p prog -l rom.layout -i normal -i "gfx_rom" -w some.rom"
+.sp
+Overlapping regions are resolved in an implementation-dependent manner (or may even yield an error) - do
+.BR "not " "rely on it."
+.sp
 .B "flashrom -h"
 lists all supported programmers.
 .TP
Index: flashrom-layout_read_region_from_file/layout.c
===================================================================
--- flashrom-layout_read_region_from_file/layout.c	(Revision 1757)
+++ flashrom-layout_read_region_from_file/layout.c	(Arbeitskopie)
@@ -33,6 +33,7 @@
    chipoff_t end;
    unsigned int included;
    char name[256];
+	char *file;
 } romentry_t;

/* rom_entries store the entries specified in a layout file and associated run-time data */
@@ -54,7 +55,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;
    }
@@ -63,8 +64,7 @@
    	char *tstr1, *tstr2;

if (num_rom_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);
    		return 1;
    	}
    	if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name))
@@ -85,6 +85,7 @@
    	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;
+		rom_entries[num_rom_entries].file = NULL;
    	num_rom_entries++;
    }

@@ -100,17 +101,6 @@
 }
 #endif

-/* returns the index of the entry (or a negative value if it is not found) */
-int find_include_arg(const char *const name)
-{
-	unsigned int i;
-	for (i = 0; i < num_include_args; i++) {
-		if (!strcmp(include_args[i], name))
-			return i;
-	}
-	return -1;
-}
-
 /* register an include argument (-i) for later processing */
 int register_include_arg(char *name)
 {
@@ -124,11 +114,6 @@
    	return 1;
    }

-	if (find_include_arg(name) != -1) {
-		msg_gerr("Duplicate region name: "%s".\n", name);
-		return 1;
-	}
-
    include_args[num_include_args] = name;
    num_include_args++;
    return 0;
@@ -138,14 +123,9 @@
 static int find_romentry(char *name)
 {
    int i;
-
-	if (num_rom_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;
+		if (strcmp(rom_entries[i].name, name) == 0) {
    		msg_gspew("found.\n");
    		return i;
    	}
@@ -165,28 +145,69 @@
    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 file is loaded. */
    if (num_rom_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(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(name);
+		if (idx < 0) {
+			msg_gerr("Nonexisting region name specified: "%s".\n", name);
+			return 1;
+		}
+		if (rom_entries[idx].included == 0) {
+			rom_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;
+			}
+			rom_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 < num_rom_entries; i++)
+		if (rom_entries[i].included) {
+			msg_gdbg(""%s"", rom_entries[i].name);
+			if (rom_entries[i].file)
+				msg_gdbg(":"%s"", rom_entries[i].file);
+			if (--found)
+				msg_gdbg(", ");
+	}
+	msg_gdbg(".\n");
    return 0;
 }

@@ -200,6 +221,8 @@
    num_include_args = 0;

for (i = 0; i < num_rom_entries; i++) {
+		free(rom_entries[i].file);
+		rom_entries[i].file = NULL;
    	rom_entries[i].included = 0;
    }
    num_rom_entries = 0;
@@ -283,6 +306,14 @@
    	if (entry->start > start)
    		memcpy(newcontents + start, oldcontents + start,
    		       entry->start - start);
+		/* If a file name is specified for this region, read the file contents and
+		 * overwrite @newcontents in the range specified by @entry. */
+		if (entry->file != NULL) {
+			if (read_buf_from_file(newcontents + entry->start, entry->end - entry->start + 1,
+			    entry->file, "the region's size") != 0)
+				return 1;
+		}
+
    	/* Skip to location after current romentry. */
    	start = entry->end + 1;
    	/* Catch overflow. */
Index: flashrom-layout_read_region_from_file/flashrom.c
===================================================================
--- flashrom-layout_read_region_from_file/flashrom.c	(Revision 1757)
+++ flashrom-layout_read_region_from_file/flashrom.c	(Arbeitskopie)
@@ -1165,8 +1165,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");
@@ -1186,8 +1185,10 @@
    	return 1;
    }
    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);
    	fclose(image);
    	return 1;
    }
@@ -1968,7 +1969,7 @@
    }

if (write_it || verify_it) {
-		if (read_buf_from_file(newcontents, size, filename)) {
+		if (read_buf_from_file(newcontents, size, filename, "the flash chip's size")) {
    		ret = 1;
    		goto out;
    	}
@@ -2002,7 +2003,12 @@
    msg_cinfo("done.\n");

/* Build a new image taking the given layout into account. */
-	build_new_image(flash, oldcontents, newcontents);
+	if (build_new_image(flash, oldcontents, newcontents)) {
+		msg_gerr("Could not prepare the data to be written due to problems with the layout,\n"
+			 "aborting.\n");
+		ret = 1;
+		goto out;
+	}

// ////////////////////////////////////////////////////////////