On Tue, 2 Aug 2011 14:48:15 -0700 David Hendricks dhendrix@google.com wrote:
The patch is mostly ack-able, save for a few nits.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
My implementation does not defer the processing until doit(), but after the argument parsing loop only (doit() should not contain argument checks).
Just FYI -- The reason we deferred processing until doit() was because our usage case calls for reading the ROM content to find pre-existing mappings (e.g. fmap) which exist in the ROM image. It might be useful to eventually do the same thing for cbfs and Intel's flash descriptor.
jup i am aware of that. but i dont like to inflate doit() with even more stuff, that does not belong there. we need to think about refactoring the whole processing... we need something like pre-doit() before doit() but after init and unlocking. a bit like the shutdown process with the registration of some callbacks... i hope work on libflashrom will enforce us to move forward with that.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
This allows to specify -i and -l parameters in any order.
I don't like the output in error cases much, any ideas? example of a good run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org
Using region(s): "normal", "gfxrom", "fallback". Calibrating delay loop... OK. Found SST flash chip "SST25VF032B" (4096 kB, SPI) on dummy. Reading old flash chip contents... done. Erasing and writing flash chip... Erase/write done. Verifying flash... VERIFIED.
Looks okay to me. And IIRC -V will list all ROM layout entries found even if they are not included, which is nice for debugging.
that was printed at default verbosity before, and is now spew-verbose (which is now at -VVV level). but yes i wanted to keep something for debugging.
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
example of a semi-good run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org
Maximum number of ROM images (4) in layout file reached before end of layout file. Ignoring the rest of the layout file.
I think that if the user accidentally specifies an invalid layout or number of included regions, then Flashrom should quit. Otherwise Flashrom might not flash everything the user intends, leaving the content in an inconsistent (probably bricked) state. This has become problematic due to complicated ROM layouts in systems where multiple masters use a shared SPI flash (e.g. an EC or Intel's ME insanity).
yes, that's certainly a well argued proposal imo. carl-daniel?
On Mon, Aug 1, 2011 at 6:05 PM, Stefan Tauner < stefan.tauner@student.tuwien.ac.at> wrote:
example of a faulty run: flashrom -p dummy:emulate=SST25VF032B -i normal -w ../testimages/4096kB.rand.img -l test.layout -i gfxrom -i fallback -i bios flashrom v0.9.4-r1396 on Linux 2.6.35-30-generic (x86_64), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org
Maximum number of ROM images (4) in layout file reached before end of layout file. Ignoring the rest of the layout file. Using region(s): "normal", "gfxrom", "fallback"Invalid region specified: "bios"
If you split that last line so that the "Invalid region" part shows up on a newline, it looks okay.
that is one option yes. but i tend to think we may defer the output of the used regions and push it into its own loop at the end of process_include_args. this was there would be no "using region..." output in case of errors at all. and it may even be more readable(?)
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
cli_classic.c | 11 ++++----- flash.h | 2 + layout.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 67 insertions(+), 14 deletions(-)
diff --git a/cli_classic.c b/cli_classic.c index 36fe9ad..e44ea86 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -233,14 +233,9 @@ int cli_classic(int argc, char *argv[]) cli_classic_abort_usage(); break; case 'i':
/* FIXME: -l has to be specified before -i. */ tempstr = strdup(optarg);if (find_romentry(tempstr) < 0) {fprintf(stderr, "Error: image %s not foundin "
"layout file or -i specified before"
"-l\n", tempstr);
if (register_include_arg(tempstr) < 0) cli_classic_abort_usage();
} break; case 'L': if (++operation_specified > 1) {@@ -325,6 +320,10 @@ int cli_classic(int argc, char *argv[]) cli_classic_abort_usage(); }
if (process_include_args() < 0) {cli_classic_abort_usage();}/* FIXME: Print the actions flashrom will take. */ if (list_supported) {diff --git a/flash.h b/flash.h index 5b49e9d..0848255 100644 --- a/flash.h +++ b/flash.h @@ -251,6 +251,8 @@ int print(int type, const char *fmt, ...) __attribute__((format(printf, 2, 3))); int cli_classic(int argc, char *argv[]);
/* layout.c */ +int register_include_arg(char *name); +int process_include_args(void); int read_romlayout(char *name); int find_romentry(char *name); int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *newcontents); diff --git a/layout.c b/layout.c index d719a05..936e316 100644 --- a/layout.c +++ b/layout.c @@ -41,6 +41,12 @@ typedef struct { char name[256]; } romlayout_t;
+/* include_args lists 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.
- */
+static char *include_args[MAX_ROMLAYOUT]; +static int num_include_args = 0; /* the number of valid entries. */ static romlayout_t rom_entries[MAX_ROMLAYOUT];
#if CONFIG_INTERNAL == 1 /* FIXME: Move the whole block to cbtable.c? */ @@ -194,6 +200,20 @@ int read_romlayout(char *name) } #endif
+/* register an include argument (-i) for later processing */ +int register_include_arg(char *name) +{
if (num_include_args >= MAX_ROMLAYOUT) {msg_gerr("Too many regions included (%i).\n",num_include_args);
return -1;}include_args[num_include_args] = name;num_include_args++;return num_include_args;+}
+/* returns the index of the entry (or a negative value if it is not found) */ int find_romentry(char *name) { int i; @@ -201,20 +221,49 @@ int find_romentry(char *name) if (!romimages) return -1;
msg_ginfo("Looking for \"%s\"... ", name);
msg_gspew("Looking for region \"%s\"... ", name); for (i = 0; i < romimages; i++) { if (!strcmp(rom_entries[i].name, name)) { rom_entries[i].included = 1;
msg_ginfo("found.\n");
msg_gspew("found.\n"); return i; } }
msg_ginfo("not found.\n"); // Not found. Error.
msg_gspew("not found.\n"); return -1;}
+/* process -i arguments
- returns 0 to indicate success, <0 to indicate failure
I think the usual Flashrom convention is to return 1 to indicate failure. I used <0 out of habit. Let's go ahead and fix that coding convention for upstream if desired, and update the call sites to do error checking correctly.
well -1 is also used very often, but that's no argument to introduce more inconsistencies :) i'll fix this, thanks.
-
Stefan Tauner