Skip to content

Comments

Audit panics path in embedded-service type-c#657

Merged
jerrysxie merged 5 commits intoOpenDevicePartnership:mainfrom
jerrysxie:panic-audit-embedded-service-type-c
Dec 22, 2025
Merged

Audit panics path in embedded-service type-c#657
jerrysxie merged 5 commits intoOpenDevicePartnership:mainfrom
jerrysxie:panic-audit-embedded-service-type-c

Conversation

@jerrysxie
Copy link
Contributor

@jerrysxie jerrysxie commented Dec 22, 2025

This pull request improves error handling and robustness in port event management by introducing explicit error types and propagating errors when invalid port indices are used. The changes ensure that port operations such as marking ports as pending or clearing them now return results, making it easier to handle invalid operations gracefully. The updates also modify existing code and tests to handle these results appropriately.

Port Event Error Handling Improvements:

  • Added a new Error enum to embedded-service/src/type_c/event.rs to represent event-related errors, such as InvalidPort.
  • Updated PortPending methods (pend_port, pend_ports, clear_port, is_pending) to return Result types and check for invalid port indices, returning Error::InvalidPort when appropriate.
  • Modified the PortPendingIter iterator and FromIterator implementation to handle and propagate errors from the updated methods [1] [2].

Controller and Service Integration:

  • Updated calls to pend_port in type-c-service/src/wrapper/mod.rs and type-c-service/src/wrapper/vdm.rs to handle errors, mapping them to existing error types where necessary [1] [2] [3].
  • Improved the Device::lookup_global_port method in embedded-service/src/type_c/controller.rs to use safe access and error propagation when looking up ports.

Test Adjustments:

  • Updated all test usages of pend_port to handle the new Result return type by unwrapping the result, ensuring tests fail on error [1] [2] [3] [4] [5] [6] [7].

* Add additional error type for event port

* Make event port methods return result instead of panicking for
  invalid port
@jerrysxie jerrysxie self-assigned this Dec 22, 2025
Copilot AI review requested due to automatic review settings December 22, 2025 00:22
@jerrysxie jerrysxie added the enhancement New feature or request label Dec 22, 2025
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 refactors event port handling in the embedded-service type-c module by replacing panicking code with proper error handling. The changes convert methods that previously could panic on invalid port indices to return Result types, allowing calling code to handle errors gracefully.

Key changes:

  • Introduced a new Error enum in the event module for invalid port handling
  • Updated PortPending methods (pend_port, pend_ports, clear_port, is_pending) to return Result<T, Error> instead of panicking
  • Modified all call sites in the wrapper and controller modules to handle the new error returns

Reviewed changes

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

File Description
embedded-service/src/type_c/event.rs Introduces Error enum and converts PortPending methods to return Results; updates FromIterator and Iterator implementations to handle errors
embedded-service/src/type_c/controller.rs Refactors lookup_global_port to use ok_or pattern instead of explicit bounds checking
type-c-service/src/wrapper/vdm.rs Updates pend_port call to handle Result and map error to PdError::InvalidPort
type-c-service/src/wrapper/mod.rs Updates two pend_port calls to handle Results and map errors to PdError::InvalidPort

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

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 5 out of 5 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.

asasine
asasine previously approved these changes Dec 22, 2025
Copy link

@asasine asasine left a comment

Choose a reason for hiding this comment

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

non-blocking suggestions

* Rename error type and add more info to error

* Make pend_ports() swallow the invalid port and log error instead of
  shortcircuiting and return an error
Copilot AI review requested due to automatic review settings December 22, 2025 18:58
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 6 out of 6 changed files in this pull request and generated 2 comments.


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

@jerrysxie jerrysxie enabled auto-merge (squash) December 22, 2025 23:25
@jerrysxie jerrysxie disabled auto-merge December 22, 2025 23:43
@jerrysxie jerrysxie added the BREAKING CHANGE Marks breaking changes label Dec 22, 2025
@jerrysxie
Copy link
Contributor Author

Announced breaking changes on Zulip: #embedded-controller > embedded-service type-c API changes @ 💬.

@jerrysxie jerrysxie enabled auto-merge (squash) December 22, 2025 23:45
@jerrysxie jerrysxie merged commit b136133 into OpenDevicePartnership:main Dec 22, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BREAKING CHANGE Marks breaking changes enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants