Improve panic handling#1264
Improve panic handling#1264berlin-with0ut-return wants to merge 7 commits intoOpenDevicePartnership:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1233 by converting various assert! macros, unwrap() calls, and similar panic-inducing patterns to proper error handling with debug_assert! and error returns where appropriate. The changes aim to ensure production code returns errors rather than panicking, while maintaining debug assertions to catch unexpected conditions during development.
Changes:
- Converted multiple
assert!statements toif+panic!with descriptive error messages for invariant violations - Replaced
assert!statements with proper error handling (debug_assert!+log::error!+ error returns) for recoverable errors - Updated
unwrap()calls to useexpect()with descriptive messages or proper error propagation
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/patina_ffs/src/volume.rs | Converted assert to debug_assert + error handling for pad file alignment; changed unwrap to ok()? in test helper |
| sdk/patina/src/uefi_protocol/decompress.rs | Replaced null pointer asserts with proper validation and error return |
| sdk/patina/src/runtime_services.rs | Converted assert to panic for null pointer validation; replaced assert with debug_assert + error handling for buffer size check |
| sdk/patina/src/pi/serializable.rs | Changed unwrap to expect with descriptive message |
| sdk/patina/src/driver_binding.rs | Converted unwrap to ok_or for null pointer handling in driver binding callbacks |
| sdk/patina/src/boot_services.rs | Replaced assert_eq with explicit null pointer check and error return |
| patina_dxe_core/src/systemtables.rs | Converted asserts to separate if + panic statements for null pointer checks |
| patina_dxe_core/src/protocol_db.rs | Updated assert to expect with message; enhanced panic message for well-known handle validation |
| patina_dxe_core/src/pi_dispatcher/image.rs | Improved assert_eq to if + panic with descriptive message |
| patina_dxe_core/src/pecoff/error.rs | Added new error variant for relocation block mismatch |
| patina_dxe_core/src/pecoff.rs | Converted assert to debug_assert + error handling for relocation validation |
| patina_dxe_core/src/misc_boot_services.rs | Changed assert to early return with logging; converted asserts to panics with improved messages |
| patina_dxe_core/src/lib.rs | Converted entry point asserts to separate if + panic checks; added error handling for HOB list setting |
| patina_dxe_core/src/gcd.rs | Replaced asserts with descriptive panic messages for memory validation |
| patina_dxe_core/src/filesystems.rs | Converted assert to proper error check and return |
| patina_dxe_core/src/events.rs | Changed TPL validation asserts to if + panic with detailed messages |
| patina_dxe_core/src/allocator/fixed_size_block_allocator.rs | Combined dual asserts into single if + panic; changed unwrap to ok_or for error handling |
| patina_dxe_core/src/allocator.rs | Replaced assert_ne with debug_assert + error return; converted assert to if + panic for stack validation |
| core/patina_internal_cpu/src/paging/x64.rs | Modified unwrap error handling pattern |
| components/patina_adv_logger/src/memory_log.rs | Updated unwrap error handling patterns |
| components/patina_adv_logger/src/logger.rs | Added condition check with debug_assert and error logging for memory log initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sdk/patina/src/boot_services.rs
Outdated
| debug_assert!(false, "`locate_protocol` returned a null pointer for protocol {:?}", protocol_guid); | ||
| log::error!("`locate_protocol` returned a null pointer for protocol {:?}", protocol_guid); | ||
| Err(efi::Status::NOT_FOUND) | ||
| } else { | ||
| Ok(()) |
There was a problem hiding this comment.
The logic has been inverted from the original code. The original assertion assert_eq!(ptr::null_mut(), interface_ptr) expected the interface pointer to be null for marker protocols and would return Ok(()). The new code returns an error when the pointer is null and Ok(()) when it's not null, which is the opposite behavior. The condition should be inverted: return Ok(()) when interface_ptr.is_null() is true, and return an error when it's not null.
| debug_assert!(false, "`locate_protocol` returned a null pointer for protocol {:?}", protocol_guid); | |
| log::error!("`locate_protocol` returned a null pointer for protocol {:?}", protocol_guid); | |
| Err(efi::Status::NOT_FOUND) | |
| } else { | |
| Ok(()) | |
| Ok(()) | |
| } else { | |
| debug_assert!( | |
| false, | |
| "`locate_protocol` returned a non-null pointer for marker protocol {:?}", | |
| protocol_guid | |
| ); | |
| log::error!( | |
| "`locate_protocol` returned a non-null pointer for marker protocol {:?}", | |
| protocol_guid | |
| ); | |
| Err(efi::Status::NOT_FOUND) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…turn/patina into panic_handling
| let data_offset = size_of::<AdvLoggerMessageEntry>() as u16; | ||
| let unaligned_size = data_offset as u32 + log_entry.data.len() as u32; | ||
| let message_size = align_up(unaligned_size, 8).unwrap() as u32; | ||
| let message_size = align_up(unaligned_size, 8)? as u32; |
There was a problem hiding this comment.
I think the as 32 is not necessary since message_size is already u32.
| assert!(self.set_hob_list(hob_list).is_ok()); | ||
| if self.set_hob_list(hob_list).is_err() { | ||
| log::error!("HOB list was already set!"); | ||
| debug_assert!(false, "HOB list was already set!"); |
There was a problem hiding this comment.
is it safe to proceed if this fails? I'm not sure how to reason about the state of the core if this fails do to some kind of weird reentrancy.
| @@ -603,7 +603,11 @@ impl Volume { | |||
|
|
|||
| //Per spec, max required_content_alignment is pad files is 16M (2^24). That means that pad file size | |||
There was a problem hiding this comment.
| //Per spec, max required_content_alignment is pad files is 16M (2^24). That means that pad file size | |
| //Per spec, max required_content_alignment of pad files is 16M (2^24). That means that pad file size |
Description
Addresses #1233.
This PR improves panic handling and logging for error cases. Some panics have been converted to errors, while others have simply been improved in clarity.
How This Was Tested
cargo make allIntegration Instructions
I would consider this more of a first pass. There may have been some panics that I missed, and possibly some panics that have been kept that would be better as Result/Options.