patina_adv_logger: Refactor core logic from component logic#1323
patina_adv_logger: Refactor core logic from component logic#1323cfernald wants to merge 1 commit intoOpenDevicePartnership:mainfrom
Conversation
Currently the adv logger crate exposes all of the component and integration test, etc. by default. However, for consumers wanting to log to the advanced from more minimal environments, this is not ideal. This commit refactors this base support to be the minimal core logic, and then a component feature on top used by DXE. Base: Exposes the core logger logic, and nothing else. Does no use alloc. Component: Exposes the component, protocol, and integrations test logic. To achieve this, this refactors the write and reader into separate modules and simplifies to use a read-only and write-only paradigm to simplify the wrapper logic. With this change, consumers can use patina_adv_logger without default features to get a minimal advanced logger implementation.
github-actions
bot
added
the
impact:non-functional
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| @@ -0,0 +1,310 @@ | |||
| //! UEFI Advanced Logger Reader Support | |||
There was a problem hiding this comment.
What's the use case for the reader? Tools/tests? In FW, other than tools and the FileLogger, we only ever write, right?
There was a problem hiding this comment.
There are two use cases at the moment. The integration test, which uses it to verify the log! and protocol logs are both making it to the memory log, and the parser which is available in the std build, and is used to build the executable parser in this crate.
The reason I'm exposing it is basically as you mentioned, giving the option for object level parsing in external consumers, but it seems appropriate to expose IMO.
| pub mod logger; | ||
|
|
||
| #[cfg(feature = "reader")] | ||
| pub mod reader; |
There was a problem hiding this comment.
cargo doc --no-default-features fails:
error[E0432]: unresolved import `crate::reader`
--> components\patina_adv_logger\src\parser.rs:11:12
|
11 | use crate::reader::AdvancedLogReader;
| ^^^^^^ could not find `reader` in the crate root
|
note: found an item that was configured out
--> components\patina_adv_logger\src\lib.rs:19:9
|
18 | #[cfg(feature = "reader")]
| ------------------ the item is gated behind the `reader` feature
19 | pub mod reader;
| ^^^^^^
error[E0432]: unresolved import `alloc`
--> components\patina_adv_logger\src\parser.rs:12:5
|
12 | use alloc::format;
| ^^^^^ help: a similar path exists: `std::alloc`
The parser module depends on reader::AdvancedLogReader but reader is gated here behind the reader feature. This fails without the reader feature being enabled. You can change how you like, but this should run successfully.
| pub struct AdvLoggerInfoV5 { | ||
| /// Signature 'ALOG' | ||
| signature: u32, | ||
| pub signature: u32, |
There was a problem hiding this comment.
I have not fully read through the PR but does every field really need to be public versus pub(crate)?
| buffer | ||
| } | ||
|
|
||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Is this really considered dead code given it is in the tests module and called by tests?
|
|
||
| const TEST_DATA_SIZE: usize = 128; | ||
|
|
||
| fn create_buffer_v5(timer_frequency: u64, hw_port_disabled: bool) -> Box<[u8]> { |
There was a problem hiding this comment.
create_buffer_v5() and create_buffer_v6() exist in both reader.rs and writer.rs. Can a common test module share code for both?
4 participants
Description
Currently the adv logger crate exposes all of the component and integration test, etc. by default. However, for consumers wanting to log to the advanced from more minimal environments, this is not ideal. This commit refactors this base support to be the minimal core logic, and then a component feature on top used by DXE.
Base: Exposes the core logger logic, and nothing else. Does no use alloc.
Component: Exposes the component, protocol, and integrations test logic.
To achieve this, this refactors the write and reader into separate modules and simplifies to use a read-only and write-only paradigm to simplify the wrapper logic.
With this change, consumers can use patina_adv_logger without default features to get a minimal advanced logger implementation.
Issue #1318
How This Was Tested
Integration Instructions
N/A