Attention is currently required from: Ricardo Quesada, Jack Rosenthal. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57397 )
Change subject: elogtool: add "add" command ......................................................................
Patch Set 1:
(8 comments)
File util/cbfstool/elogtool.c:
https://review.coreboot.org/c/coreboot/+/57397/comment/0e935d64_743aa5b4 PS1, Line 155: offset = sizeof(struct elog_header); : data = buffer_get(buf); : : /* Point to the first event */ : event = data + offset; : : while (offset < bytes_to_shrink) { : assert(!(event->type == ELOG_TYPE_EOL || event->length == 0) && : "Unexpected premature end of buffer"); : offset += event->length; : event = elog_get_next_event(event); : } : cleared = offset - sizeof(struct elog_header); : remaining = buffer_size(buf) - offset; : : /* Overlapping copy */ : memmove(data + hdr_size, data + hdr_size + cleared, remaining); : memset(data + hdr_size + remaining, ELOG_TYPE_EOL, cleared); Use of buffer operations should get rid of some of the math:
``` size_t cleared = 0; struct buffer rb, *r_iter = &rb; struct buffer wb, *w_iter = &wb;
buffer_clone(r_iter, buf); buffer_clone(w_iter, buf);
while (cleared < bytes_to_shrink) {
assert(buffer_size(r_iter) > sizeof(struct event_header)); event = buffer_get(r_iter);
assert(event->type != ELOG_TYPE_EOL && event->length > 0 && event->length <= buffer_size(r_iter));
buffer_seek(r_iter, event->length); cleared += event->length; }
memmove(buffer_get(w_iter), buffer_get(r_iter), buffer_size(r_iter)); buffer_seek(w_iter, buffer_size(r_iter)); memset(buffer_get(w_iter), ELOG_TYPE_EOL, buffer_size(w_iter));
if (!eventlog_init_event(buffer_get(w_iter), ELOG_TYPE_LOG_CLEAR, cleared, sizeof(cleared)) return ELOGTOOL_EXIT_NOT_ENOUGH_BUFFER_SPACE;
*out_offset = next_available_event_offset(w_iter); return ELOGTOOL_EXIT_SUCCESS;
```
https://review.coreboot.org/c/coreboot/+/57397/comment/6f72ddc0_8001629d PS1, Line 247: nit: The usage() function above prints out the invoked name as well. I think this should too for consistency and also how typically usage functions are written.
https://review.coreboot.org/c/coreboot/+/57397/comment/17639ef6_0e64b250 PS1, Line 290: can Did you mean cannot?
https://review.coreboot.org/c/coreboot/+/57397/comment/9c77f624_a7f137a4 PS1, Line 310: int size_t
https://review.coreboot.org/c/coreboot/+/57397/comment/b4bc41ae_71fb9382 PS1, Line 312: int size_t
https://review.coreboot.org/c/coreboot/+/57397/comment/fc33d818_09fdad8c PS1, Line 313: int size_t
https://review.coreboot.org/c/coreboot/+/57397/comment/f40535ea_041b8814 PS1, Line 314: int size_t
https://review.coreboot.org/c/coreboot/+/57397/comment/d4f1320c_ce25a996 PS1, Line 323: As commented on https://review.coreboot.org/c/coreboot/+/57395/1/util/cbfstool/elogtool.c#13..., I think we can seek past the header here to keep things simpler for rest of the functions.
``` buffer_clone(©, b); buffer_seek(©, sizeof((struct elog_header));
offset = next_available_event_offset(©); if (offset > threshold) { ret = shrink_buffer(©, buffer_size(©) - threshold, &offset); if (ret ...) ... }
buffer_seek(©, offset); ... ```