Nico Huber has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom) ......................................................................
Patch Set 24:
(5 comments)
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@52 PS22, Line 52: --fmap\fR|\fB--fmap-file\fR <file>) \
Yeah, it's not the longest line but it is quite long. […]
Sorry, I meant the visible output in a man page viewer. You can see above that some lines are already broken manually for the output (when a linebreak is not escaped).
So the whole thing could become
.B flashrom \fR[\fB-h\fR|\fB-R\fR|\fB-L\fR|\fB-z\fR|\fB-p\fR <programmername>[:<parameters>] [\fB-E\fR|\fB-r\fR <file>|\fB-w\fR <file>|\fB-v\fR <file>] [\fB-c\fR <chipname>] [(\fB-l\fR <file>|\fB--ifd\fR|\fB--fmap\fR|\fB--fmap-file\fR <file>) [\fB-i\fR <image>]] [\fB-n\fR] [\fB-N\fR] [\fB-f\fR]] [\fB-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>]
which should be output as follows
flashrom [-h|-R|-L|-z|-p <programmername>[:<parameters>] [-E|-r <file>|-w <file>|-v <file>] [-c <chipname>] [(-l <file>|--ifd|--fmap|--fmap-file <file>) [-i <image>]] [-n] [-N] [-f]] [-V[V[V]]] [-o <logfile>]
Note that there also was a \fR missing after --ifd.
https://review.coreboot.org/#/c/23203/22/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/22/fmap.c@203 PS22, Line 203: }
moved above malloc
Now the malloc error path is missing the finalize_flash_access()...
https://review.coreboot.org/#/c/23203/22/fmap.c@321 PS22, Line 321: an fma
I say "an" in my head. […]
Thanks.
AFAIK, it's not literally about a consonant vs. a vowel, rather about the sound you make. Even with a literal vowel you might make a consonant sound and vice versa, e.g. unique, hour...
https://review.coreboot.org/#/c/23203/24/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/24/fmap.c@281 PS24, Line 281: * should be a valid, usable fmap. */ Should we set `ret = 2` here too?
https://review.coreboot.org/#/c/23203/24/fmap.c@285 PS24, Line 285: *fmap_out = malloc(fmap_len); : if (*fmap_out == NULL) { : msg_gerr("Out of memory.\n"); : goto _free_ret; : } : : memcpy(*fmap_out, fmap, fmap_len); Now that the code is more straight forward to read. We don't need this malloc()/memcpy(), do we? Wouldn't a simple `*fmap_out = fmap;` suffice (while skipping the free(fmap) ofc)?
-
Nico Huber (Code Review)