Attention is currently required from: Furquan Shaikh, Jack Rosenthal. Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56883 )
Change subject: elogtool: add "clear" command ......................................................................
Patch Set 3:
(12 comments)
Patchset:
PS3: Thanks. Fixed!
Created a new CL for the bcd code. It is the ancestor of this CL.
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/4730440b_cf355940 PS2, Line 50: mybin2bcd
I think you can just move the function to common bsd code instead of worrying about gpl implications […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/08c44844_6c21f9f3 PS2, Line 57:
indentation nit: since there's stuff on the line above beyond the (, you need to indent to the (
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/b8510480_991f8201 PS2, Line 66: sanity
drop "sanity" (inclusive language)
Done
File src/commonlib/bsd/include/commonlib/bsd/elog.h:
https://review.coreboot.org/c/coreboot/+/56883/comment/6d9b2207_d79e6cf4 PS2, Line 317:
same indentation comment as above
Done
File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/67828539_5ef700e9 PS2, Line 793: 0
spaces inside of braces
Done
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/191ffb99_93849da2 PS2, Line 134: + 1
using <= you can drop the +1
the "+ 1" is for the checksum byte, so in total header + data + 1 is needed.
(added "byte" word next to "checksum for clarity)
https://review.coreboot.org/c/coreboot/+/56883/comment/61c9e9e4_4ac4a942 PS2, Line 144: (const char *)(event)
is the cast necessary?
yes, without it I get: "error: comparison of distinct pointer types lacks a cast"
https://review.coreboot.org/c/coreboot/+/56883/comment/2f20e6d4_1108a839 PS2, Line 155:
indentation
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/1f1f4c01_08cdfd1f PS2, Line 166: NULL
no input file option for clear command?
added. input file gets overwritten (following the same pattern as when no file is provided). Although not sure whether you'd like to have an output file.
https://review.coreboot.org/c/coreboot/+/56883/comment/a70a2f93_fa5362f6 PS2, Line 176: "RW_ELOG"
since we've used this constant twice now, would be nice to make a #define for it
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/9806c1b6_60ed484d PS2, Line 177: (void *)
nit: api expects a uint8_t *
Done