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 29:
(3 comments)
Patchset:
PS27:
In the follow up CB:52362 I implement the chromiumos syntax in https://flashrom.org/Per_region_file_arguments
Not exactly. Chromium flashrom still allows the filename to be anywhere in the command line.
If this "feature" is not needed anymore, that's very good news. Would have been even better to know 3 years ago, that would have spared us a lot of work and discussions.
Convergence has advanced quite a bit (crrev.com/c/2823343) that I think striving for the same CLI syntax is the right approach. Also, all of these changes are backward compatible with existent interface.
No, they are not. Upstream flashrom is a bit more subtle. We have a lot of very humble users. Flashrom's text output is also a part of the user interface. Currently we can clearly say, for instance, `-r is missing an argument'. With optional file name arguments both for `-i` and for `-r/-v/-w`, it's not that clear anymore. This is what CB:30979 tries to address. I don't insist on it. But it would be nice to have talked about it.
File cli_classic.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/2e733e66_75fc9285 PS27, Line 319: tempstr = strdup(optarg);
strdup() here since strtok() modifies the string: […]
Yes, I know, the string needs to be duplicated somewhere. However, there is no reason to let the caller do it. It's just historical burden if kept here.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/23021/comment/b03383b6_2909363b PS16, Line 1367: when writing
I have been working with the intention of reduce as much as possible the differences between upstrea […]
Thanks.
I know it's a lot of work. But whenever diffing upstream and chromium one should at least check for every hunk which version is newer. We have it too often that good upstream changes are effectively reverted by chromium upstreaming. Fixing that alone costs the community a lot of time, letting spare-time reviewers do the checking increases this cost tremendously. In some cases I even had to call it hostile. So far Google shows no intention (beside empty promises) to ever make up for this.
Sorry for the lament, it's been going on for years and is just very tiresome. It would be nice to make all changes due to diffing the trees visible.
-
Nico Huber (Code Review)