Attention is currently required from: Sam McNally, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger.
8 comments:
Patchset:
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:
Patch Set #27, 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:
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.
Patch Set #16, Line 1367: when writing
Any other case when this is used? or, why remove this?
File layout.c:
Patch Set #21, 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:
Patch Set #27, Line 147: tmp->file = strdup(file);
else?
Patch Set #27, Line 155: include_region
Could be exported as part of the libflashrom API, e.g.
flashrom_layout_include_region_file()
Looks like somewhere in the rebasing reporting of the file names got lost.
To view, visit change 23021. To unsubscribe, or for help writing mail filters, visit settings.