Attention is currently required from: Ricardo Quesada. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56883 )
Change subject: elogtool: add "clear" command ......................................................................
Patch Set 4:
(14 comments)
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/c8882363_28194bed PS4, Line 50: I think it would be best to split this CL into two:
1. Add/move function definitions to bsd/elog.c 2. Update elogtool to support clear subcommand
Easier to review and also easier to bisect and identify any regressions later.
https://review.coreboot.org/c/coreboot/+/56883/comment/0bcceaa6_e4dff9ca PS4, Line 51: /* Populate timestamp in event header with given time. */ I think these comments would be very helpful in elog.h.
https://review.coreboot.org/c/coreboot/+/56883/comment/bdc0a68e_be05c207 PS4, Line 82: struct const
https://review.coreboot.org/c/coreboot/+/56883/comment/07f7919d_8f9e5e74 PS4, Line 85: uint8_t const
File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/f9a748ca_4f069855 PS4, Line 821: #if CONFIG(RTC) I don't think this needs to be a CPP check: ``` if (CONFIG(RTC)) rtc_get(&time); ``` should work.
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/f539351d_153226b6 PS4, Line 16: #define RW_ELOG_REGION "RW_ELOG" I think this should also be added to bsd/elog.h. It is required by both elog driver in coreboot and the tool.
https://review.coreboot.org/c/coreboot/+/56883/comment/d7688565_2474cc06 PS4, Line 133: data_size = buf->size - sizeof(struct elog_header); : data_offset = buf->data + sizeof(struct elog_header); ``` struct buffer event_data_buf; struct buffer *b = &event_data_buf;
buffer_clone(b, buf); buffer_seek(b, sizeof(struct elog_header)); ``` Gets rid of some of the math that needs to be done explicitly here and also can use the buffer pointer for other cases below.
https://review.coreboot.org/c/coreboot/+/56883/comment/319777a4_6abfe2de PS4, Line 137: if (data_size < sizeof(struct event_header) + sizeof(used_data_size) + 1) : return ELOGTOOL_EXIT_INVALID_ELOG_FORMAT; Rather than adding this check here, I think it would be helpful to just pass in the buffer to `eventlog_init_event()` so that it can check that there is enough buffer size before writing the new event to it. It can be useful for "add" subcommand as well.
https://review.coreboot.org/c/coreboot/+/56883/comment/1214c6ed_ccbb98ed PS4, Line 145: (const struct event_header *) I don't think this typecast is required.
``` event = buffer_get(b); ```
https://review.coreboot.org/c/coreboot/+/56883/comment/ad63a3b8_6d946da0 PS4, Line 147: buf->data + buf->size I think you can add a helper for this in a separate change: `buffer_end()`
https://review.coreboot.org/c/coreboot/+/56883/comment/52039714_b6067b45 PS4, Line 175: ELOGTOOL_EXIT_BAD_INPUT_PATH If the read was successful but write back failed, it might not really be a problem with bad path?
https://review.coreboot.org/c/coreboot/+/56883/comment/88b937a9_e9157e5d PS4, Line 181: (uint8_t *) buf->data, buf->size ``` buffer_get(buf), buffer_size(buf)); ```
https://review.coreboot.org/c/coreboot/+/56883/comment/0dc34175_e5304cf7 PS4, Line 194: /* Returned buffer must be freed. */ : struct buffer buf; : ret = elog_read(filename, &buf); : if (ret != 0) : return ret; : I think this operation is now going to be required for each elog subcommand. We should move this to be handled by the caller and also introduce a command table to handle the commands and write back at the caller: ``` int main(...) { ...
struct buffer buf; ret = elog_read(filename, &buf); if (ret) return ret;
for (i = 0; i < ARRAY_SIZE(cmds); i++) { if (!strcmp(cmds[i].name, argv[optind]) ret = cmds[i].func(&buf); }
if (i == ARRAY_SIZE(cmds)) ret = ELOGTOOL_EXIT_BAD_ARGS;
if (cmds[i].write_back) save_buffer(&buf, filename);
buffer_delete(&buf); return ret; } ```
`write_back` will be set by add/clear but not by list.
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/71d3bacf_f9da1094 PS4, Line 654: (uint32_t *)&event[1]; event_get_data(event);