Change in ...flashrom[master]: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/30979
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
[RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
In case the user only wants to specify files for specific layout regions, we need a way to set the operation without specifying a file. Instead of making the <filename> argument optional, we can add a new syntax for this particular purpose. This way, we avoid complex command line parsing and can do more sanity checks and provide better error messages.
TODO: Update manpage in case this gets accepted.
Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b Signed-off-by: Nico Huber nico.huber@secunet.com --- M cli_classic.c 1 file changed, 42 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/30979/1
diff --git a/cli_classic.c b/cli_classic.c index 324e145..d09a8d1 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -39,7 +39,8 @@ "-z|" #endif "-p <programmername>[:<parameters>] [-c <chipname>]\n" - "[-E|(-r|-w|-v) <file>] [(-l <layoutfile>|--ifd) [-i <imagename>]...] [-n] [-N] [-f]]\n" + "[(-r|-w|-v|-E) <file>|-m (r|w|v|e)] [(-l <layoutfile>|--ifd) [-i <region>]...]\n" + "[-n] [-N] [-f]]\n" "[-V[V[V]]] [-o <logfile>]\n\n", name);
printf(" -h | --help print this help text\n" @@ -47,6 +48,8 @@ " -r | --read <file> read flash and save to <file>\n" " -w | --write <file> write <file> to flash\n" " -v | --verify <file> verify flash against <file>\n" + " -m | --mode <operation> for use with --include <region>:<file>,\n" + " specify the mode of operation (r|w|v|e)\n" " -E | --erase erase flash memory\n" " -V | --verbose more verbose output\n" " -c | --chip <chipname> probe only for specified flash chip\n" @@ -123,6 +126,7 @@ {"write", 1, NULL, 'w'}, {"erase", 0, NULL, 'E'}, {"verify", 1, NULL, 'v'}, + {"mode", 1, NULL, 'm'}, {"noverify", 0, NULL, 'n'}, {"noverify-all", 0, NULL, 'N'}, {"chip", 1, NULL, 'c'}, @@ -200,6 +204,25 @@ filename = strdup(optarg); verify_it = 1; break; + case 'm': + if (++operation_specified > 1) { + fprintf(stderr, "More than one operation specified. Aborting.\n"); + cli_classic_abort_usage(); + } + /* check that `optarg` is a prefix of a known operation */ + if (strncmp(optarg, "read", strlen(optarg)) == 0) { + read_it = 1; + } else if (strncmp(optarg, "write", strlen(optarg)) == 0) { + write_it = 1; + } else if (strncmp(optarg, "verify", strlen(optarg)) == 0) { + verify_it = 1; + } else if (strncmp(optarg, "erase", strlen(optarg)) == 0) { + erase_it = 1; + } else { + fprintf(stderr, "Unknown operation specified: '%s'\n", optarg); + cli_classic_abort_usage(); + } + break; case 'n': if (verify_it) { fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n"); @@ -404,7 +427,7 @@ cli_classic_abort_usage(); }
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) { + if (filename && check_filename(filename, "image")) { cli_classic_abort_usage(); } if (layoutfile && check_filename(layoutfile, "layout")) { @@ -654,6 +677,23 @@ flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it); flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all);
+ if (!filename && (read_it | write_it | verify_it)) { + if (layout->num_entries == 0) { + msg_gerr("Error: The specified operation requires a file.\n"); + ret = 1; + goto out_shutdown; + } + for (i = 0; i < layout->num_entries; i++) { + if (!layout->entries[i].file) { + msg_gerr("Error: Region "%s" requires a file argument.\n", + layout->entries[i].name); + 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.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/30979/1/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/30979/1/cli_classic.c@42 PS1, Line 42: (-r|-w|-v|-E) Separate change?
Hello Paul Menzel, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/30979
to look at the new patch set (#2).
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
[RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter
In case the user only wants to specify files for specific layout regions, we need a way to set the operation without specifying a file. Instead of making the <filename> argument optional, we can add a new syntax for this particular purpose. This way, we avoid complex command line parsing and can do more sanity checks and provide better error messages.
TODO: Update manpage in case this gets accepted.
Change-Id: Idfba11ec9991aac423b07f68f7dc45e7bebbb06b Signed-off-by: Nico Huber nico.huber@secunet.com --- M cli_classic.c 1 file changed, 43 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/30979/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/30979/1/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/30979/1/cli_classic.c@42 PS1, Line 42: (-r|-w|-v|-E)
Separate change?
Yes, thought about it :) will do if we ever agree on a command-line syntax.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
This might make it more confusing for the moment, my goal would be to have something like
flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1
for individual regions, and
flashrom -p internal -i all:some.rom --mode write #2
for the whole flash, where
flashrom -p internal -w some.rom #3
would be a shorthand for #2.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
This might make it more confusing for the moment, my goal would be to have something like
flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1for individual regions, and
flashrom -p internal -i all:some.rom --mode write #2for the whole flash, where
flashrom -p internal -w some.rom #3would be a shorthand for #2.
In any case, this is a step in the right direction by making full-sized filename optional. I'll be completely onboard if `flashrom -p internal --ifd -i bios:cbfs.bin -w` can be shorthand for #1 and `flashrom -p internal -i all:some.rom -w` can be shorthand for #2.
Otherwise, we should should deprecate -w/-r/-E/-v to be consistent for the various use cases.
https://review.coreboot.org/#/c/30979/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30979/2//COMMIT_MSG@14 PS2, Line 14: provide better error messages. I'm not sure what is so complex... Optional arguments are well-supported by getopt_long() and non-positional filename was how flashrom originally worked.
Some day I'll get around to rebasing my patches to demonstrate this more clearly...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
This might make it more confusing for the moment, my goal would be to have something like
flashrom -p internal --ifd -i bios:cbfs.bin --mode write #1for individual regions, and
flashrom -p internal -i all:some.rom --mode write #2for the whole flash, where
flashrom -p internal -w some.rom #3would be a shorthand for #2.
In any case, this is a step in the right direction by making full-sized filename optional. I'll be completely onboard if `flashrom -p internal --ifd -i bios:cbfs.bin -w` can be shorthand for #1 and `flashrom -p internal -i all:some.rom -w` can be shorthand for #2.
Well, I added a short form `-m` and a prefix check, so `-mw` would work. `-w` does still ask for a file name.
Otherwise, we should should deprecate -w/-r/-E/-v to be consistent for the various use cases.
I thought about that, too. But the old syntax is really handy when it comes to flashing full files :-/
https://review.coreboot.org/#/c/30979/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30979/2//COMMIT_MSG@14 PS2, Line 14: provide better error messages.
I'm not sure what is so complex... […]
The parsing part is solved, Arthur looked after that in 23022. Not easy to read, but acceptable.
But the optional arguments make it hard to guess what the user intended, e.g. I think currently it would tell the user to specify files for each region even if he maybe just forgot to specify the full flash-chip file. Having different syntaxes for each case evades this problem.
Attention is currently required from: David Hendricks, Daniel Campello, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
I believe the main difference between this approach and CB:52362 is that this -m argument is only ne […]
TBH, I'm not very convinced of either version. One downside of the optional argument that I probably didn't repeat lately: Typos in the command line can become more severe (but not as bad as with the non-positional argument). For instance, consider this:
$ flashrom -p prog --ifd -i bios:bios.bin -r
And then you want to add a `-V` but miss the dash, it would still be accepted:
$ flashrom -p prog --ifd -i bios:bios.bin -r V
It's not very bad. IIRC, with the non-positional argument one could do much funnier things.
On the wiki[1], I once came up with this:
[-i <region>]... (-r|-w|-v [<region>:]<filename>)...
I'm not sure anymore, why I implemented `--mode` instead. It seems the classic CLI will grow further anyway. If we'd start with this:
[-i <region>[:<file>]]... -r|-w|-v [<filename>]
We could still later add a (-r|-w|-v [[<region>:]<filename>]) notation. Then we'd stay Cr compatible but nobody would have to use the error-prone optional argument. It would also be easier to have compile-time switches, e.g. to make the file argument optional. If we'd decide to go this way, I'd prefer to have only the latter syntax added to the help text / man page.
What do you think? The idea is rather new to me, I still haven't made up my mind.
[1] https://flashrom.org/Per_region_file_arguments#More_explicit_alternative
Attention is currently required from: Nico Huber, David Hendricks, Edward O'Callaghan. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/30979/comment/b0e6c807_62ace6b7 PS2, Line 14: provide better error messages.
I have to say I didn't really like dealing with the optional argument in the cros tree David however […]
I'm not sure how the combinatorics are smaller in this case. I believe is the same amount of cases as I presented on my previous message.
Re error reporting we can make it clear that both options exist: "Missing region filename for region <region> when no flash_chip file was provided with <operation>"
I still think that the UX around keeping the same flag for the same operation (regardless of the input/output artifacts) is much easier to reason about from the user perspective. Having -r <file> and -m r[ead] options for reading flash chip in my opinion is confusing. Just to reiterate what David said optional arguments are not outside of standard as they are supported by getopt.
Said all of that, if the consensus is for this option I am fine with it.
Attention is currently required from: Nico Huber, David Hendricks. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/30979/comment/b0840b24_3d010793 PS2, Line 14: provide better error messages.
The parsing part is solved, Arthur looked after that in […]
I have to say I didn't really like dealing with the optional argument in the cros tree David however I couldn't think of a better alternative.
I much much prefer Nico's idea of a mode here, the combinatorics of options is much smaller and therefore the behavior is much easier to reason about.
+1'ed as I just want to make sure Daniel Campello is on board for this?
Attention is currently required from: Nico Huber, David Hendricks, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Yeah, I remember that was a concern. […]
I just thought of something that may be confusing with the latest proposal:
# flashrom -r file # when no -i arg reads the whole chip # flashrom -i region -r file # reads only region <region> on the ROM size file with everything else 0 # flashrom -r region:region_file file # is not obvious what file may have since no -i was provided
I think given the above it is much cleaner to keep the -i region:file interface.
Attention is currently required from: Nico Huber, Edward O'Callaghan. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/30979/comment/534642d8_056984a5 PS2, Line 687: layout->entries[i].file Here you could do check_filename(layout->entries[i].file, "region") for additional checks (i.e. != '\0')
Attention is currently required from: Nico Huber, Edward O'Callaghan. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2: I believe the main difference between this approach and CB:52362 is that this -m argument is only needed if no -r/-v/-w <file> is specified but not needed/allowed otherwise.
With this apporach: - If someone wants to read a region *and* the whole chip: # flashrom -i region:region-file.bin -r chip-file.bin - If someone wants to read a region *and not* the whole chip: # flashrom -i region:region-file.bin -m r
With CB:52362 - If someone wants to read a region *and* the whole chip: # flashrom -i region:region-file.bin -r chip-file.bin - If someone wants to read a region *and not* the whole chip: # flashrom -i region:region-file.bin -r
I *personally* think having -r do the signaling for both types of reads is cleaner given that -m won't be used for "normal" full chip reads.
As an a side (not that I'm saying this should be a deciding factor for upstream direction): the second approach should be compatible with chromium usage and will keep both CLIs almost ("almost" chromium has a couple of additional flags --do-not-diff and --ignore-lock) aligned as of today.
Attention is currently required from: Nico Huber. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2: Code-Review+1
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
- Backwards-compatible with flashrom syntax prior to CB:23021 […]
Yeah, I remember that was a concern. Does anybody remember exactly why? IIRC it was considered complicated, yet non-positional files have been supported and standard in Unix commands since before most of us were born so I have a hard time understanding that argument.
Using non-positional files as illustrated in https://flashrom.org/Per_region_file_arguments#Use_cases enables us to treat chip-sized files and region-sized files with the same, intuitive syntax without added mode flags or inconsistent use of -i/-r/-w/-v as in other proposals.
Attention is currently required from: Nico Huber, David Hendricks, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
- Backwards-compatible with flashrom syntax prior to CB:23021
For it to be backwards compatible the file argument may appear anywhere is the cmdline:
# flashrom -r file -V --noverify-all # flashrom -r -V --noverify-all file # flashrom -r -V file --noverify-all
I believe this was the main complain of the chromium syntax on CB:23022 which I am addressing in CB:52362 (and crrev.com/c/2823343)
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2: I came up with another option that I think addresses many of the concerns raised here: https://flashrom.org/Per_region_file_arguments#Alternative_.233
Advantages: - No positional or optional argument after -r/-w/-v. This simplifies them and makes them consistent with -E, so this approach *actually* reduces combinatorics. - -r/-w/-v is specified exactly once. - No added modes. - Arguments are handled similarly to standard Unix commands (ls, rm, cat, etc) - Backwards-compatible with flashrom syntax prior to CB:23021
LMK what you think.
Attention is currently required from: Nico Huber, Edward O'Callaghan, Daniel Campello, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2: Code-Review-1
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/30979/comment/6e27cbd3_d06a4e27 PS2, Line 14: provide better error messages.
I'm not sure how the combinatorics are smaller in this case. I believe is the same amount of cases as I presented on my previous message.
Re error reporting we can make it clear that both options exist: "Missing region filename for region <region> when no flash_chip file was provided with <operation>"
I still think that the UX around keeping the same flag for the same operation (regardless of the input/output artifacts) is much easier to reason about from the user perspective. Having -r <file> and -m r[ead] options for reading flash chip in my opinion is confusing. Just to reiterate what David said optional arguments are not outside of standard as they are supported by getopt.
Said all of that, if the consensus is for this option I am fine with it.
I still don't like the proposed syntax, but if others feel really strongly in favor then I won't block it.
I agree with Daniel's analysis above. I don't buy the argument that adding another parameter will somehow decrease combinatorics - I think it will add complexity to the code and make usage even more confusing. Perhaps I'm missing the bigger picture though.
BTW, to illustrate the UX I've added a table to the wiki: https://flashrom.org/Per_region_file_arguments#Use_cases
Let's fill that out and see if that helps persuade folks one way or the other.
One thing we might do to address Nico's comment about unintentionally using the optional argument in CrOS-style syntax is make it so one can either operate on regions *or* operate on a full-sized file, but not both at the same time. So for example, remove the ability to do: flashrom -p prog -i region:region.bin -r file.bin flashrom -p prog -i region:region.bin -w file.bin
IIRC those were optimizations for ChromeOS factory scripts. Err'ing out in those cases will address the concern Nico had about unintentionally using the optional argument, though it will cost a few seconds on the manufacturing line.
Patchset:
PS2:
And then you want to add a `-V` but miss the dash, it would still be accepted:
$ flashrom -p prog --ifd -i bios:bios.bin -r V
It's not very bad. IIRC, with the non-positional argument one could do much funnier things.
Agree here! At least it seems to me that it just a concern for --read (which makes it non-dangerous), since for the other two (-w/-v) it would be a lot of bad luck to have a filename called V (still possible) while making the typo.
Flashrom will just err out if the file isn't the same size as the flash chip.
Actually, this use case is (was?) an optimization for parts of the manufacturing process where we wanted to flash the final coreboot image and patch certain regions with data generated at manufacturing time. Not very user-friendly, but it saved some time. I'd be fine with err'ing out in upstream if folks think it's prone to misuse/errors.
On the wiki[1], I once came up with this:
[-i <region>]... (-r|-w|-v [<region>:]<filename>)...
I think the complication with this approach is to support multiple file regions in a single operation. We would have to enforce multiple -r without the possibility of mixes between -r/-w/-v... Also, it means that we would have to move away form -i <region>:<filename> (or as you suggested bellow - not document it). Furthermore -i would only make sense if there is a chip size operation since otherwise is already embedded on the -r/-w/-v option. As an example of something that would not make much sense:
$ flashrom -i region1 -r region2:file # excluding a -r chip_file
Yeah, we should stick with one way to specify regions. Deprecating `-i` and only using `-r|-w|-v [<region>:]<filename>` could work, though as you say we need to ensure that only one type of operation is used for the entire command.
PS2: I'm not convinced that adding another syntax for operations involving regions is a net benefit, but I won't stand in the way if you all think this is definitely an improvement.
IMO if we're going to propose a new syntax, then let's think it through carefully so that it handles all cases elegantly.
Attention is currently required from: Nico Huber, David Hendricks, Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30979 )
Change subject: [RFC] cli_classic: Add `--mode (read|write|verify|erase)` parameter ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
And then you want to add a `-V` but miss the dash, it would still be accepted:
$ flashrom -p prog --ifd -i bios:bios.bin -r V
It's not very bad. IIRC, with the non-positional argument one could do much funnier things.
Agree here! At least it seems to me that it just a concern for --read (which makes it non-dangerous), since for the other two (-w/-v) it would be a lot of bad luck to have a filename called V (still possible) while making the typo.
On the wiki[1], I once came up with this:
[-i <region>]... (-r|-w|-v [<region>:]<filename>)...
I think the complication with this approach is to support multiple file regions in a single operation. We would have to enforce multiple -r without the possibility of mixes between -r/-w/-v... Also, it means that we would have to move away form -i <region>:<filename> (or as you suggested bellow - not document it). Furthermore -i would only make sense if there is a chip size operation since otherwise is already embedded on the -r/-w/-v option. As an example of something that would not make much sense:
$ flashrom -i region1 -r region2:file # excluding a -r chip_file
[1] https://flashrom.org/Per_region_file_arguments#More_explicit_alternative
I probably should have clicked the link before starting replying as my concerns are documented there :)
-
Daniel Campello (Code Review) -
David Hendricks (Code Review)
-
Edward O'Callaghan (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)