Attention is currently required from: Furquan Shaikh. Ricardo Quesada has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56883 )
Change subject: elogtool: add "clear" command ......................................................................
Patch Set 5:
(17 comments)
Patchset:
PS5: fixed. could you PTALA? ty!
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/51b80d41_32c98859 PS1, Line 50: static inline uint8_t mybin2bcd(uint8_t val) : { : return ((val / 10) << 4) | (val % 10); : } :
Already defined here: […]
Done
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/56c13852_b657feb4 PS2, Line 79: void elog_update_checksum(struct event_header *event, uint8_t checksum) : { : uint8_t *event_data = (uint8_t *)event; : event_data[event->length - 1] = checksum; : }
assuming that the two checksum functions where created by Googler's and it is Ok to relicense them u […]
Done
File src/commonlib/bsd/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/f62f795b_516541d9 PS4, Line 50:
I think it would be best to split this CL into two: […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/54161087_416bb84c PS4, Line 51: /* Populate timestamp in event header with given time. */
I think these comments would be very helpful in elog.h.
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/c231c0a9_42343924 PS4, Line 82: struct
const
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/eb74f58b_bc6b67e1 PS4, Line 85: uint8_t
const
Done
File src/drivers/elog/elog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/35a2fb21_6e29b888 PS4, Line 821: #if CONFIG(RTC)
I don't think this needs to be a CPP check: […]
Done
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/a3bcf6c8_40fb1bf4 PS4, Line 16: #define RW_ELOG_REGION "RW_ELOG"
I think this should also be added to bsd/elog.h. […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/15614a6e_33485c5f PS4, Line 133: data_size = buf->size - sizeof(struct elog_header); : data_offset = buf->data + sizeof(struct elog_header);
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/bcf24100_0551aece 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 `event […]
do you mean replacing the `struct event header*` with ` struct buffer*`? e.g:
void eventlog_init_event(struct buffer* buf, uint8_t type, ....)
?
I can do that, but looks kind-of-strange to pass a "struct buffer*" to a function that expects a event_header*.
Perhaps a I can create an additional function that calls the original one. e.g:
void eventlog_init_event_with_buf(struct buffer* buf, ....) { /* check size */ void* b = buffer_get(buf); return eventlog_init_event(b, ...); }
WDYT?
https://review.coreboot.org/c/coreboot/+/56883/comment/7e0ade74_dbefb8a1 PS4, Line 145: (const struct event_header *)
I don't think this typecast is required. […]
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/4b3b2b20_cc6fc399 PS4, Line 147: buf->data + buf->size
I think you can add a helper for this in a separate change: `buffer_end()`
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/f8d0f07f_bd20fd1d 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?
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/8353e507_1dff4ed8 PS4, Line 181: (uint8_t *) buf->data, buf->size
Done
https://review.coreboot.org/c/coreboot/+/56883/comment/05fa5d3d_29129061 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. […]
yay, much better!
File util/cbfstool/eventlog.c:
https://review.coreboot.org/c/coreboot/+/56883/comment/84e5eb7b_778ea91f PS4, Line 654: (uint32_t *)&event[1];
event_get_data(event);
good point.
Should I rename the current `event_get_data()` to `event_get_data_const()` (which receives and returns a `const*`), and add a new one that receives and returns a `non-const*` ?