Skip to content

Comments

embedded-service cfu: audit panics#656

Merged
jerrysxie merged 3 commits intoOpenDevicePartnership:mainfrom
jerrysxie:panic-audit-embedded-service-cfu
Dec 22, 2025
Merged

embedded-service cfu: audit panics#656
jerrysxie merged 3 commits intoOpenDevicePartnership:mainfrom
jerrysxie:panic-audit-embedded-service-cfu

Conversation

@jerrysxie
Copy link
Contributor

@jerrysxie jerrysxie commented Dec 20, 2025

This pull request introduces improvements to error handling and code safety in the CFU component and device request logic. The main changes ensure that protocol errors are properly surfaced when sub-component information is missing, and refactor device lookup logic to use more idiomatic Rust patterns, reducing the risk of panics and improving code clarity.

Error Handling and Protocol Safety

  • When retrieving firmware version information from sub-components in CfuComponentDefault, the code now checks for missing data and returns a protocol error (CfuProtocolError::BadResponse) instead of using a default value. This prevents silent failures and makes error cases explicit.

Code Refactoring and Idiomatic Rust

  • Refactored device lookup in route_request, send_device_request, and wait_device_response functions to use if let Some(device) instead of manually unwrapping Option. This eliminates potential panics and makes the code more idiomatic and readable.

Safety and Documentation

  • Added a compile-time assertion to guarantee that MAX_CMPT_COUNT is always one greater than MAX_SUBCMPT_COUNT, ensuring safe indexing and preventing out-of-bounds errors.
  • Improved comments and annotations to clarify panic safety and suppress unnecessary Clippy warnings where indexing is guaranteed to be safe.

@jerrysxie jerrysxie self-assigned this Dec 20, 2025
@jerrysxie jerrysxie added the enhancement New feature or request label Dec 20, 2025
Copilot AI review requested due to automatic review settings December 20, 2025 01:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves error handling and panic safety in the CFU (Component Firmware Update) service by replacing manual unwrapping with idiomatic pattern matching and documenting indexing safety.

Key changes:

  • Refactored route_request, send_device_request, and wait_device_response functions to use if let Some(device) pattern matching instead of manual unwrapping, eliminating potential panics
  • Added clippy allow attribute and safety comment for array indexing operation in component.rs

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
embedded-service/src/cfu/mod.rs Refactored three routing functions to use pattern matching for Option handling, making error handling more explicit and idiomatic while eliminating unwrap() calls
embedded-service/src/cfu/component.rs Added clippy::indexing_slicing allow attribute with safety comment to document why array indexing is safe in the FwVersionRequest handler

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings December 20, 2025 01:22
@jerrysxie jerrysxie marked this pull request as ready for review December 20, 2025 01:22
@jerrysxie jerrysxie requested a review from a team as a code owner December 20, 2025 01:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kurtjd
kurtjd previously approved these changes Dec 20, 2025
kurtjd
kurtjd previously approved these changes Dec 22, 2025
Copilot AI review requested due to automatic review settings December 22, 2025 21:05
@jerrysxie jerrysxie force-pushed the panic-audit-embedded-service-cfu branch from 45d828b to 718e4c3 Compare December 22, 2025 21:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

embedded-service/src/cfu/component.rs:264

  • The error handling in this branch seems inconsistent with the changes in the if branch above. When a sub-component returns an unexpected response type (not a FwVersionResponse), this code silently creates dummy firmware version info instead of returning a protocol error. This contradicts the PR's stated goal of ensuring protocol errors are properly surfaced instead of using default values. Consider returning CfuError::ProtocolError(CfuProtocolError::BadResponse) here as well to maintain consistency.
                        } else {
                            /*error!(
                                "Failed to get firmware version from sub-component: {}, adding dummy info to list",
                                id
                            );*/
                            comp_info[index + 1] = FwVerComponentInfo::new(FwVersion::default(), index as u8 + 1);
                        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jerrysxie jerrysxie merged commit abf6f0d into OpenDevicePartnership:main Dec 22, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants