openhcl: keep logging levels and emit them in usermode #2618
Conversation
|
This PR modifies files containing For more on why we check whole files, instead of just diffs, check out the Rustonomicon |
There was a problem hiding this comment.
Pull request overview
This PR updates the string_page_buf crate to persist and emit log levels alongside log messages. Previously, the buffer stored only concatenated UTF-8 strings; now it stores structured log entries with level information (Error/Warn/Info/Debug/Trace) encoded as a byte followed by message length and content.
Key changes:
- Introduces a structured entry format with 3-byte headers (level + u16 length) per log message
- Replaces the
append()method withappend_log()that acceptsfmt::Argumentsand a log level - Updates consumers (openhcl_boot and underhill_core) to use the new entry iterator API and emit logs at appropriate levels
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| support/string_page_buf/src/lib.rs | Core changes: adds EntryHeader struct, level encoding functions, LogEntry/LogEntryIter types, replaces append() with append_log(), adds SliceWriter for efficient formatting, and comprehensive test updates |
| support/string_page_buf/Cargo.toml | Adds log crate dependency for Level type |
| openhcl/underhill_core/src/loader/vtl2_config/mod.rs | Creates BootshimLogEntry struct to store level+message, updates bootshim log reading to iterate entries, maps log levels to appropriate tracing macros |
| openhcl/openhcl_boot/src/boot_logger.rs | Refactors Logger to implement Write trait directly, updates log() to use append_log(), changes runtime init to iterate and emit entries with levels |
| Cargo.lock | Updates dependency graph with log crate for string_page_buf |
| if bytes_written > u16::MAX as usize { | ||
| self.header.dropped = self.header.dropped.saturating_add(1); | ||
| return Err(StringBufferError::StringTooLong); | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the case where a formatted message exceeds u16::MAX bytes (65535). While this is unlikely in practice, the code path exists and should be tested to verify the error handling behavior is correct.
| let total_len = ENTRY_HEADER_SIZE + bytes_written; | ||
| self.header.next_insert += total_len as u16; |
There was a problem hiding this comment.
Potential arithmetic overflow when incrementing next_insert. The code computes total_len = ENTRY_HEADER_SIZE + bytes_written where bytes_written can be up to u16::MAX. Since ENTRY_HEADER_SIZE is 3, total_len could be up to 65538 (u16::MAX + 3), which exceeds u16::MAX and will truncate when cast to u16 on line 335. This could corrupt the buffer state.
The check on line 322 should account for ENTRY_HEADER_SIZE, i.e., check if ENTRY_HEADER_SIZE + bytes_written > u16::MAX instead of just bytes_written > u16::MAX.
| } | ||
|
|
||
| if bytes_written > u16::MAX as usize { | ||
| self.header.dropped = self.header.dropped.saturating_add(1); |
There was a problem hiding this comment.
There's an inconsistency in error handling when the message exceeds u16::MAX bytes. The code increments dropped counter and then returns an error. However, this is inconsistent with the other error paths which only increment dropped and return Ok(false).
Either the function should return Err(StringBufferError::StringTooLong) without incrementing dropped (treating it as a validation error), or it should return Ok(false) like other space-related failures (treating it as a drop). The current behavior is confusing because it both counts the message as dropped AND returns an error.
| self.header.dropped = self.header.dropped.saturating_add(1); |
1 participant
After switching to the log crate, we now have log levels associated with each log call. Persist them by updating the
string_page_bufstructure to store a log level with the log entry, and update callers to use that info.