Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support ......................................................................
Patch Set 30:
(2 comments)
Patchset:
PS27:
In the follow up CB:52362 I implement the chromiumos syntax in https://flashrom. […]
Looking at the current code in chromium (crrev.com/c/2823343):
if (read_it || write_it || verify_it) { if (argv[optind]) filename = argv[optind]; }
It is currently implemented (similar to CB:23022) as the -r/-v/-w can be anywhere in the cmdline but filename actually has to be after all flag arguments. With my change in CB:52362 (and its counterpart crrev.com/c/2823343 ) this changes to filename has to follow (optionally) -r/-v/-w (which is how any normal flag would behave). I have been running tests and scouting on the chromiumos code base for current usage and I'm hoping that it would not be a breaking change. If anything, I think that having upstream flashrom behave this way and have chromiumos behave both ways (in case of breaking change) is a good middle ground where to meet.
On CB:52362 you would get a "Error: No image file specified." for missing all -r/-v/-w/-i filenames (which is the existing message for missing -r/-v/-w on check_filename()) and a "Error: No region file specified." if missing all -r/-v/-w filenames and some -i filenames. Error reporting level is maintained.
One thing to note is that this patch in itself does not change the CLI handling (and error handling) for -r/-v/-w arguments as they are still required.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/d98d5042_a2dc63fb PS27, Line 319: tempstr = strdup(optarg);
Yes, I know, the string needs to be duplicated somewhere. However, there is […]
Fixed!
-
Daniel Campello (Code Review)