Attention is currently required from: Ricardo Quesada, Jack Rosenthal. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56883 )
Change subject: elogtool: add "clear" command ......................................................................
Patch Set 6:
(8 comments)
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/4eb9da06_647e8297 PS4, Line 137: if (data_size < sizeof(struct event_header) + sizeof(used_data_size) + 1) : return ELOGTOOL_EXIT_INVALID_ELOG_FORMAT;
do you mean replacing the `struct event header*` with ` struct buffer*`? e.g: void eventlog_init_event(struct buffer* buf, uint8_t type, ....)
Yes, basically updating the signature of `eventlog_init_event()` to take in a buffer that points to the area where event needs to be added.
I can do that, but looks kind-of-strange to pass a "struct buffer*" to a function that expects a event_header*.
Once you update the signature, the function would be expecting a `struct buffer *` or do you mean eventlog_* functions in general expecting a `struct event_header *`? I think it should be okay to just update the function to use `struct buffer *`. Easier to do the bound checking. In fact, it makes sense to pass in `struct buffer *` even for `eventlog_print_event()` [that should help address comment from Julius about bound checking for print operation too]. But, the print change can be done as a separate CL.
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/6ba3c689_147968eb PS6, Line 29: static static const
https://review.coreboot.org/c/coreboot/+/56883/comment/78421e8a_0b3c4e4f PS6, Line 57: RW_ELOG nit: Use `ELOG_RW_REGION_NAME`?
https://review.coreboot.org/c/coreboot/+/56883/comment/db6b4efa_0a76547b PS6, Line 82: /* else */ I think the comment can be dropped.
https://review.coreboot.org/c/coreboot/+/56883/comment/773d7a65_4305797e PS6, Line 96: int ret; nit: ret is not really required, unless you are planning to print it in fprintf on L102.
https://review.coreboot.org/c/coreboot/+/56883/comment/4d08960e_35b6bf43 PS6, Line 159: buffer_seek(b, sizeof(struct elog_header)); Don't you need to initialize elog_header before doing the seek operation?
https://review.coreboot.org/c/coreboot/+/56883/comment/24552743_55b63a43 PS6, Line 171: (const void *) I don't think the typecast is required.
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/8ee7929f_e5a62c3e PS4, Line 654: (uint32_t *)&event[1];
good point. […]
I think we can just drop the `const` in the return value and let the caller use a const if required. We don't really need two separate helper functions(const and non-const versions) in my opinion.