Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/37240 )
Change subject: cbtable.c: don't assume high addresses can fully map 1 MiB ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
Just some comments and nits. We allow up to 112 chars per line if it increases readability. I think that's the case for all the prematurely wrapped lines here.
https://review.coreboot.org/c/flashrom/+/37240/1/cbtable.c File cbtable.c:
https://review.coreboot.org/c/flashrom/+/37240/1/cbtable.c@231 PS1, Line 231: addr Can't we just use `offset`? It seems to be the better name. I was confused by `addr` in about every other line mentioning it below.
https://review.coreboot.org/c/flashrom/+/37240/1/cbtable.c@255 PS1, Line 255: return NULL; Why not continue with the original base?
It seems inconsistent because we continue searching if lb_header_valid() && !lb_table_valid() but not if lb_header_valid() && "bad length/mapping"?
https://review.coreboot.org/c/flashrom/+/37240/1/cbtable.c@259 PS1, Line 259: head = (struct lb_header *)(((char *)base) + addr); Move this inside the `if` so it becomes clear that it's for the updated `base`?