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 <[email protected]> >> Signed-off-by: Stefan Tauner <[email protected]> > 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 <[email protected]> > Signed-off-by: Stefan Tauner <[email protected]> > Signed-off-by: Carl-Daniel Hailfinger <[email protected]>
Latest version. Signed-off-by: Carl-Daniel Hailfinger <[email protected]> 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; + } // //////////////////////////////////////////////////////////// -- http://www.hailfinger.org/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
