Attention is currently required from: Sam McNally, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 )
Change subject: layout: Add -i <region>[:<file>] support ......................................................................
Patch Set 27:
(8 comments)
Patchset:
PS27: Thanks for taking this over. I have to say, however, that it was only stalled because of the follow-up(s) to adapt the CLI, where we couldn't find a consensus yet. I think we really should have a discussion about the command-line syntax, and that should happen on the mailing list. Some pointers for a start:
CB:23022 CB:30979 https://flashrom.org/Per_region_file_arguments https://mail.coreboot.org/pipermail/flashrom/2018-January/015327.html
It seems some code was lost in the rebase PS16 and re-invented later. So I'm not really sure how to re-review this. I tried to diff various patch sets to figure out why what changed since PS15, but it's really hard. If we have to start over, I'd prefer smaller patches. For instance, the internals in `flashrom.c` could be done first. With just NULL file pointers in the romentries, it would even be a consistent program.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/4f94262c_c9c25943 PS27, Line 319: tempstr = strdup(optarg); If register_include_arg() is not going to own it, we should pass it `const` and handle the duplication inside.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/12b74e22_253b9897 PS16, Line 1337: %s
Done
Still, somebody took the time to implement better error messages. I don't see why we should drop that. But I also don't really mind.
https://review.coreboot.org/c/flashrom/+/23021/comment/2104e891_90b0b4ff PS16, Line 1367: when writing Any other case when this is used? or, why remove this?
File layout.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/3f76aefb_04f30fe7 PS21, Line 253: goto out; The intention obviously was to report all overlapping regions.
If we decide to bail out, there is no need for the variable and goto.
File layout.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/7dbf3015_103da5a9 PS27, Line 147: tmp->file = strdup(file); else?
https://review.coreboot.org/c/flashrom/+/23021/comment/0a777dbc_81acde8f PS27, Line 155: include_region Could be exported as part of the libflashrom API, e.g.
flashrom_layout_include_region_file()
https://review.coreboot.org/c/flashrom/+/23021/comment/9400830d_d51ddf70 PS27, Line 221: } Looks like somewhere in the rebasing reporting of the file names got lost.
-
Nico Huber (Code Review)