Change in coreboot[master]: cbfstool: Do host space address conversion earlier when adding files
Attention is currently required from: Patrick Georgi, Paul Menzel, Tim Wawrzynczak. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60018 )
Change subject: cbfstool: Do host space address conversion earlier when adding files ......................................................................
Patch Set 3:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/60018/comment/f65a8bef_36a77181 PS3, Line 11: but
by?
Done
https://review.coreboot.org/c/coreboot/+/60018/comment/8aa54a65_8d21cd85 PS3, Line 21: CB:59877
Could you please also use the commit hash and summary?
...why? I thought this is what we have the CB:xxx shorthand for? I find it nice that citing another patch (especially when it's more than one) doesn't always take up multiple lines in the commit message.
File util/cbfstool/cbfstool.c:
https://review.coreboot.org/c/coreboot/+/60018/comment/48c3e1dd_beae3c93 PS3, Line 813: offset
If I'm reading this right, this assumes a flash_offset is always passed in right? I don't see anythi […]
No, this is actually the function where |offset| is treated purely as an out parameter (don't confuse convert_region_offset() with convert_addr_space()... some of the naming in here could probably use some cleaning up). It doesn't matter what you pass in here (and on passing back out I believe(?) it's always flash address space, but just to be sure I left the initial conversion after this point in the code flow.
https://review.coreboot.org/c/coreboot/+/60018/comment/567513dc_e9f79f88 PS3, Line 1052: offset
One thing you could do to make this easier to read is rename `offset` to `flash_offset`, and `addres […]
Well, |offset| is how it's called in all these functions (except for convert_region_offset() which is also super confusing), I wouldn't want to change it here without changing it everywhere
-
Julius Werner (Code Review)