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.
TODO: Move the relevant man page chunk from 3/6 to 2/6. Will do so in a
followup mail.
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]>
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>]...] [-c
<chipname>]\n"
+ "[-E|(-r|-w|-v) <imagefile>] [-l <layoutfile> [-i
<region>[:<regionfile>]...]]\n"
+ " [-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/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
