Matt DeVillier has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/85159?usp=email )
Change subject: cli_classic.c: Make -r/-w/-v argument optional ......................................................................
cli_classic.c: Make -r/-w/-v argument optional
Makes filename optional from -r/-w/-v since the -i parameter allows the image to be written to be sourced from multiple files, and regions to be read from flash and written to separate image files.
Based on https://review.coreboot.org/c/flashrom/+/52362.
TEST=verify read/write/verify operations successful when specifying the filename only after the region.
Change-Id: I6eba095d478f1a7bdbc3854627a656f93dd9e452 Signed-off-by: Matt DeVillier matt.devillier@gmail.com --- M cli_classic.c M include/layout.h M layout.c 3 files changed, 94 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/85159/1
diff --git a/cli_classic.c b/cli_classic.c index 8f37019..bb42917 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -108,17 +108,17 @@ printf("Usage: %s [-h|-R|-L|" "\n\t-p <programmername>[:<parameters>] [-c <chipname>]\n" "\t\t(--flash-name|--flash-size|\n" - "\t\t [-E|-x|(-r|-w|-v) <file>]\n" + "\t\t [-E|-x|(-r|-w|-v) [<file>]]\n" "\t\t [(-l <layoutfile>|--ifd| --fmap|--fmap-file <file>) [-i <region>[:<file>]]...]\n" "\t\t [-n] [-N] [-f])]\n" "\t[-V[V[V]]] [-o <logfile>]\n\n", name);
printf(" -h | --help print this help text\n" " -R | --version print version (release)\n" - " -r | --read <file> read flash and save to <file>\n" - " -w | --write (<file>|-) write <file> or the content provided\n" + " -r | --read [<file>] read flash and save to <file>\n" + " -w | --write [<file>|-] write <file> or the content provided\n" " on the standard input to flash\n" - " -v | --verify (<file>|-) verify flash against <file>\n" + " -v | --verify [<file>|-] verify flash against <file>\n" " or the content provided on the standard input\n" " -E | --erase erase flash memory\n" " -V | --verbose more verbose output\n" @@ -616,6 +616,23 @@ return 0; }
+static char *get_optional_filename(char *argv[]) +{ + char *filename = NULL; + + /* filename was supplied in optarg (i.e. -rfilename) */ + if (optarg != NULL) + filename = strdup(optarg); + /* filename is on optind if it is not another flag (i.e. -r filename) + * - is treated as stdin, so we still strdup in this case + */ + else if (optarg == NULL && argv[optind] != NULL && + (argv[optind][0] != '-' || argv[optind][1] == '\0')) + filename = strdup(argv[optind++]); + + return filename; +} + static int do_read(struct flashctx *const flash, const char *const filename) { int ret; @@ -663,8 +680,10 @@ }
/* Read '-w' argument first... */ - if (read_buf_from_file(newcontents, flash_size, filename)) - goto _free_ret; + if (filename) { + if (read_buf_from_file(newcontents, flash_size, filename)) + goto _free_ret; + } /* * ... then update newcontents with contents from files provided to '-i' * args if needed. @@ -697,8 +716,10 @@ }
/* Read '-v' argument first... */ - if (read_buf_from_file(newcontents, flash_size, filename)) - goto _free_ret; + if (filename) { + if (read_buf_from_file(newcontents, flash_size, filename)) + goto _free_ret; + } /* * ... then update newcontents with contents from files provided to '-i' * args if needed. @@ -773,12 +794,12 @@ switch (opt) { case 'r': cli_classic_validate_singleop(&operation_specified); - options->filename = strdup(optarg); + options->filename = get_optional_filename(argv); options->read_it = true; break; case 'w': cli_classic_validate_singleop(&operation_specified); - options->filename = strdup(optarg); + options->filename = get_optional_filename(argv); options->write_it = true; break; case 'v': @@ -787,7 +808,7 @@ if (options->dont_verify_it) { cli_classic_abort_usage("--verify and --noverify are mutually exclusive. Aborting.\n"); } - options->filename = strdup(optarg); + options->filename = get_optional_filename(argv); options->verify_it = true; break; case 'n': @@ -1033,12 +1054,12 @@ int ret = 0;
struct cli_options options = { 0 }; - static const char optstring[] = "r:Rw:v:nNVEfc:l:i:p:Lzho:x"; + static const char optstring[] = "r::Rw::v::nNVEfc:l:i:p:Lzho:x"; static const struct option long_options[] = { - {"read", 1, NULL, 'r'}, - {"write", 1, NULL, 'w'}, + {"read", 2, NULL, 'r'}, + {"write", 2, NULL, 'w'}, {"erase", 0, NULL, 'E'}, - {"verify", 1, NULL, 'v'}, + {"verify", 2, NULL, 'v'}, {"noverify", 0, NULL, 'n'}, {"noverify-all", 0, NULL, 'N'}, {"extract", 0, NULL, 'x'}, @@ -1098,7 +1119,7 @@
parse_options(argc, argv, optstring, long_options, &options);
- if ((options.read_it | options.write_it | options.verify_it) && check_filename(options.filename, "image")) + if (options.filename && check_filename(options.filename, "image")) cli_classic_abort_usage(NULL); if (options.layoutfile && check_filename(options.layoutfile, "layout")) cli_classic_abort_usage(NULL); @@ -1316,6 +1337,49 @@ goto out_shutdown; }
+ /* + * 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. + * - 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 ((options.read_it | options.write_it | options.verify_it) && !options.filename) { + if (!options.include_args) { + msg_gerr("Error: No image file specified.\n"); + ret = 1; + goto out_shutdown; + } + + if (check_include_args_filename(options.include_args)) { + ret = 1; + goto out_shutdown; + } + } + if (options.flash_name) { if (fill_flash->chip->vendor && fill_flash->chip->name) { printf("vendor="%s" name="%s"\n", diff --git a/include/layout.h b/include/layout.h index d03a15b..ce3dd4b 100644 --- a/include/layout.h +++ b/include/layout.h @@ -69,6 +69,7 @@
int register_include_arg(struct layout_include_args **, const char *arg); int process_include_args(struct flashrom_layout *, const struct layout_include_args *); +int check_include_args_filename(const struct layout_include_args *); void cleanup_include_args(struct layout_include_args **);
const struct romentry *layout_next_included_region(const struct flashrom_layout *, chipoff_t); diff --git a/layout.c b/layout.c index e46e61a..5386de8 100644 --- a/layout.c +++ b/layout.c @@ -288,6 +288,19 @@ return 0; }
+int check_include_args_filename(const struct layout_include_args *include_args) +{ + const struct layout_include_args *arg; + for (arg = include_args; arg; arg = arg->next) { + if (!arg->file || (arg->file[0] == '\0')) { + fprintf(stderr, "Error: No region file specified.\n"); + return 1; + } + } + + return 0; +} + /* returns boolean 1 if any regions overlap, 0 otherwise */ int included_regions_overlap(const struct flashrom_layout *const l) {