Hello Louis Yung-Chieh Lo,
I'd like you to do a code review. Please visit
https://review.coreboot.org/19387
to review the following change.
Change subject: make args for -r/-w/-v non-positional and optional ......................................................................
make args for -r/-w/-v non-positional and optional
Ported from chromiumos: https://chromium-review.googlesource.com/#/c/60515/
This makes a filename following -r, -w, and -v operations non- positional. The first argument that does not begin with a hyphen (-) is the filename for -r/-w/-v. If no such argument exists, then -r/-w/-v options apply to files specified for individually included regions via -i.
This has a few side-effects: 1. It allows us to omit the ROM-sized filename entirely when a filename is provided for a particular region when using -i. For example, "flashrom -i FOO:foo.bin -r".
2. It allows, for better or worse, the filename be specified anywhere on the command line after the program name. Our scripts and docs should still specify the file after -r/-w/-v for clarity.
For our purposes, if a filename is given for every included region (-i region:filename) then the user does not need to specify a filename after -r/-w/-v. However, if any -i option does not specify a filename, then a file must be specified after -r/-w/-v.
The syntax will be backwards compatible for now so that one can still mix -i options with and without the added filename specifier for each region.
BUG=chromium:263495 BRANCH=none TEST=manual (see notes below)
1. Write random data to RW_UNUSED region (on snow in this case) without requiring an argument to -w. See that only RW_UNUSED is erased and written, and that verify works: dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc flashrom -p host -i RW_UNUSED:rw_unused.bin -w -V flashrom -p host -i RW_UNUSED:rw_unused.bin -v -V
2. Same as above, but specify a dummy file to test syntax backwards compatibility: dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc dd if=/dev/urandom of=random_4M.bin bs=1M count=4 flashrom -p host -i RW_UNUSED:rw_unused.bin -w random_4M.bin -V flashrom -p host -i RW_UNUSED:rw_unused.bin -v random_4M.bin -V
3. Test that dumping RW_UNUSED and GBB regions without -r arg dumps two files and that they can be used to verify the content: flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v
4. Same as above, but with dummy file: flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r x.bin flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v x.bin
Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803 Reviewed-on: https://gerrit.chromium.org/gerrit/60515 Reviewed-by: Yung-Chieh Lo yjlou@chromium.org Commit-Queue: David Hendricks dhendrix@chromium.org Tested-by: David Hendricks dhendrix@chromium.org --- M cli_classic.c M flashrom.c M layout.c M layout.h 4 files changed, 202 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/87/19387/1
diff --git a/cli_classic.c b/cli_classic.c index 0024483..e85ccbd 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -111,12 +111,12 @@ enum programmer prog = PROGRAMMER_INVALID; int ret = 0;
- static const char optstring[] = "r:Rw:v:nAVEfc:l:di:p:Lzho:"; + static const char optstring[] = "rRwvnAVEfc:l:di:p:Lzho:"; static const struct option long_options[] = { - {"read", 1, NULL, 'r'}, - {"write", 1, NULL, 'w'}, + {"read", 0, NULL, 'r'}, + {"write", 0, NULL, 'w'}, {"erase", 0, NULL, 'E'}, - {"verify", 1, NULL, 'v'}, + {"verify", 0, NULL, 'v'}, {"noverify", 0, NULL, 'n'}, {"noverify-all", 0, NULL, 'A'}, {"chip", 1, NULL, 'c'}, @@ -163,7 +163,6 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - filename = strdup(optarg); read_it = 1; break; case 'w': @@ -172,7 +171,6 @@ "specified. Aborting.\n"); cli_classic_abort_usage(); } - filename = strdup(optarg); write_it = 1; break; case 'v': @@ -186,7 +184,6 @@ fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); cli_classic_abort_usage(); } - filename = strdup(optarg); verify_it = 1; break; case 'n': @@ -344,12 +341,12 @@ } }
- if (optind < argc) { - fprintf(stderr, "Error: Extra parameter found.\n"); - cli_classic_abort_usage(); + if (read_it || write_it || verify_it) { + if (argv[optind]) + filename = strdup(argv[optind]); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if ((read_it | write_it | verify_it) && filename && check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -558,6 +555,71 @@ fl_flag_set(fill_flash, FL_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); fl_flag_set(fill_flash, FL_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all);
+ /* + * Common rules for -r/-w/-v syntax parsing: + * - If no filename is specified at all, quit. + * - If no filename is specified for -r/-w/-v, but files are specified + * for -i, then the number of file arguments for -i options must be + * equal to the total number of -i options. + * + * Rules for reading: + * - If files are specified for -i args but not -r, do partial reads for + * each -i arg, creating a new file for each region. Each -i option + * must specify a filename. + * - If filenames are specified for -r and -i args, then: + * - Do partial read for each -i arg, creating a new file for + * each region where a filename is provided (-i region:filename). + * - Create a ROM-sized file with partially filled content. For each + * -i arg, fill the corresponding offset with content from ROM. + * + * Rules for writing and verifying: + * - If files are specified for both -w/-v and -i args, -i files take + * priority. (Note: We determined this was the most useful syntax for + * chromium.org's flashrom after some discussion. Upstream may wish + * to quit in this case due to ambiguity). + * See: http://crbug.com/263495. + * - If file is specified for -w/-v and no files are specified with -i + * args, then the file is to be used for writing/verifying the entire + * ROM. + * - If files are specified for -i args but not -w, do partial writes + * for each -i arg. Likewise for -v and -i args. All -i args must + * supply a filename. Any omission is considered ambiguous. + * - Regions with a filename associated must not overlap. This is also + * considered ambiguous. Note: This is checked later since it requires + * processing the layout/fmap first. + */ + if (read_it || write_it || verify_it) { + char op = 0; + + if (read_it) + op = 'r'; + else if (write_it) + op = 'w'; + else if (verify_it) + op = 'v'; + + if (!filename) { + if (layout->num_entries == 0) { + msg_gerr("Error: No file specified for -%c.\n", op); + ret = 1; + goto out_shutdown; + } + + for (i = 0; i < layout->num_entries; i++) { + if (!layout->entries[i].included) + continue; + char *const file = layout->entries[i].file; + if (!file || !strlen(file)) { + msg_gerr("Error: Missing filename for region "%s"\n", file); + ret = 1; + } + } + + if (ret) + goto out_shutdown; + } + } + /* FIXME: We should issue an unconditional chip reset here. This can be * done once we have a .reset function in struct flashchip. * Give the chip time to settle. diff --git a/flashrom.c b/flashrom.c index 0993a07..6864b1c 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1327,6 +1327,76 @@ #endif }
+static const struct fl_layout *get_layout(const struct flashctx *const flash); +static int read_buf_from_include_args(const struct flashctx *const flash, unsigned char *buf, unsigned long size) +{ +#ifdef __LIBPAYLOAD__ + msg_gerr("Error: No file I/O support in libpayload\n"); + return 1; +#else + int ret = 0, i; + const struct fl_layout *const layout = get_layout(flash); + const struct fl_romentry *entry; + + /* + * Content will be read from -i args, so they must not overlap since + * we need to know exactly what content to write to the ROM. + */ + if (included_regions_overlap(layout)) { + msg_gerr("Error: Included regions must not overlap when writing.\n"); + return 1; + } + + /* sanity check region boundaries before accessing the chip */ + for (i = 0; i < layout->num_entries; i++) { + entry = &layout->entries[i]; + if (!entry->included) + continue; + + if ((entry->start > size) || (entry->end > size)) { + msg_gerr("Region "%s" exceeds the chip size.\n", entry->name); + return 1; + } + } + + for (i = 0; i < layout->num_entries; i++) { + FILE *image; + + entry = &layout->entries[i]; + chipoff_t len = entry->end - entry->start; + + /* TODO(dhendrix) make sure layout->entries[i].file is NULL for non-included regions */ + if (entry->included && entry->file) { + if ((image = fopen(entry->file, "rb")) == NULL) { + msg_gerr("Error: opening file "%s" failed: %s\n", entry->file, strerror(errno)); + return 1; + } + } else { + continue; + } + + struct stat image_stat; + if (fstat(fileno(image), &image_stat) != 0) { + msg_gerr("Error: getting metadata of file "%s" failed: %s\n", entry->file, strerror(errno)); + ret = 1; + goto out; + } + + unsigned long numbytes = fread(buf + entry->start, 1, len, image); + fclose(image); + if (numbytes != len) { + msg_gerr("Error: Failed to read file "%s". Got %ld bytes, " + "wanted %u!\n", entry->file, numbytes, len); + ret = 1; + break; + } + + } +out: + return ret; +#endif +} + /** * @brief Writes included layout regions into buffer(s) * @@ -1389,7 +1459,6 @@ return ret; }
-static const struct fl_layout *get_layout(const struct flashctx *const flashctx); /** * @brief Writes content from buffer to one or more files. * @@ -1417,14 +1486,18 @@
size_t i; for (i = 0; i < layout->num_entries; i++) { - if (!layout->entries[i].included) - continue; - if (!layout->entries[i].file) + if (!layout->entries[i].included || !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 chipoff_t region_end = layout->entries[i].end; + const chipsize_t region_len = region_end - region_start + 1; const char *region_file = layout->entries[i].file; + + if (i == 0) + msg_gdbg("\n"); /* avoid printing in caller's output */ + msg_gdbg("Writing region %s [0x%06x:0x%06x] to file "%s"\n", + layout->entries[i].name, region_start, region_end, region_file);
ret = do_write_buf_to_file(buf + region_start, region_len, region_file); if (ret) @@ -1441,7 +1514,6 @@ return ret; #endif } -
static int read_by_layout(struct flashctx *, uint8_t *); int read_flash_to_file(struct flashctx *flash, const char *filename) @@ -2570,8 +2642,18 @@ goto _free_ret; }
- if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size")) - goto _free_ret; + /* + * This must be done before any files specified by -i arguments are + * processed and merged into newcontents since -i files take priority. + * See http://crbug.com/263495. + */ + if (filename) { + if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size")) + goto _free_ret; + } else { + if (read_buf_from_include_args(flash, newcontents, flash_size)) + goto _free_ret; + }
ret = fl_image_write(flash, newcontents, flash_size);
diff --git a/layout.c b/layout.c index 6656b4b..2703275 100644 --- a/layout.c +++ b/layout.c @@ -229,6 +229,44 @@ return 0; }
+/* returns boolean 1 if any regions overlap, 0 otherwise */ +int included_regions_overlap(const struct fl_layout *const fl_layout) +{ + int i, overlap_detected = 0; + + for (i = 0; i < fl_layout->num_entries; i++) { + int j; + + if (!fl_layout->entries[i].included) + continue; + + for (j = 0; j < fl_layout->num_entries; j++) { + if (!fl_layout->entries[j].included) + continue; + + if (i == j) + continue; + + if (fl_layout->entries[i].start > fl_layout->entries[j].end) + continue; + + if (fl_layout->entries[i].end < fl_layout->entries[j].start) + continue; + + msg_gdbg("Regions %s [0x%08x-0x%08x] and " + "%s [0x%08x-0x%08x] overlap\n", + fl_layout->entries[i].name, fl_layout->entries[i].start, + fl_layout->entries[i].end, fl_layout->entries[j].name, + fl_layout->entries[j].start, fl_layout->entries[j].end); + overlap_detected = 1; + goto out; + } + + } +out: + return overlap_detected; +} + void layout_cleanup(void) { int i; diff --git a/layout.h b/layout.h index 439f029..19ba9f0 100644 --- a/layout.h +++ b/layout.h @@ -66,5 +66,6 @@ struct fl_layout *get_global_layout(void);
int process_include_args(struct fl_layout *); +int included_regions_overlap(const struct fl_layout *const fl_layout);
#endif /* !__LAYOUT_H__ */