support Complex errors with encoding: Zerocopy
#59
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
added a commit
to oxidecomputer/hubris
that referenced
this pull request
May 11, 2025
Currently, the `cpu-seq` API has the sequencer return an `IllegalTransition` error in the case where the requested power state is the same as the current power state. This is also the error variant returned when the requested state differs from the current state, but a transition between those two states cannot be performed. This is unfortunate, as the caller cannot distinguish between cases where the system *is* in the requested state and everything is basically fine, and cases where the caller asked the sequencer to do something that will never succeed. This commit changes the `cpu-seq` Idol API to return a `Transition` type from the `set_power_state` and `set_power_state_with_reason` IPCs that indicates whether or not a power state change occurred. Sequencer implementations are changed to return `Ok(Transition::NoChange)` instead of `Err(IllegalTransition)` when no state change occurred because the current and requested states are the same. I considered alternatively having the `IllegalTransition` error return the current state so that the caller can distinguish between illegal transitions that are totally disallowed and cases where no transition occurs. However, this requires changing the entire IPC from using `zerocopy` to `hubpack`, or adding an empty byte of padding to every *other* `SeqError` variant so that we can use the support for returning `Complex` error types with `zerocopy` I added in oxidecomputer/idolatry#59. That gets a bit trickier, as we also convert the `SeqError` code into a `u32` to send it upstack, so I figured keeping it as a C-like enum was better than adding logic to convert the error into a `u32` error code rather than just `as` casting it. Also, it felt a bit wrong for this to be an "error", in my opinion. Doing nothing because we were already in the requested state feels more like a different type of "success" to me... This is a first step towards fixing oxidecomputer/management-gateway-service#270. Fully fixing that will also require adding a way to indicate to MGS whether a state transition occurred or not, which also requires a change to the `gateway-sp-messages` crate.
hawkw
added a commit
to oxidecomputer/hubris
that referenced
this pull request
May 12, 2025
Currently, the `cpu-seq` API has the sequencer return an `IllegalTransition` error in the case where the requested power state is the same as the current power state. This is also the error variant returned when the requested state differs from the current state, but a transition between those two states cannot be performed. This is unfortunate, as the caller cannot distinguish between cases where the system *is* in the requested state and everything is basically fine, and cases where the caller asked the sequencer to do something that will never succeed. This commit changes the `cpu-seq` Idol API to return a `Transition` type from the `set_power_state` and `set_power_state_with_reason` IPCs that indicates whether or not a power state change occurred. Sequencer implementations are changed to return `Ok(Transition::NoChange)` instead of `Err(IllegalTransition)` when no state change occurred because the current and requested states are the same. I considered alternatively having the `IllegalTransition` error return the current state so that the caller can distinguish between illegal transitions that are totally disallowed and cases where no transition occurs. However, this requires changing the entire IPC from using `zerocopy` to `hubpack`, or adding an empty byte of padding to every *other* `SeqError` variant so that we can use the support for returning `Complex` error types with `zerocopy` I added in oxidecomputer/idolatry#59. That gets a bit trickier, as we also convert the `SeqError` code into a `u32` to send it upstack, so I figured keeping it as a C-like enum was better than adding logic to convert the error into a `u32` error code rather than just `as` casting it. Also, it felt a bit wrong for this to be an "error", in my opinion. Doing nothing because we were already in the requested state feels more like a different type of "success" to me... This is a first step towards fixing oxidecomputer/management-gateway-service#270. Fully fixing that will also require adding a way to indicate to MGS whether a state transition occurred or not, which also requires a change to the `gateway-sp-messages` crate.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #57 updated
idolto generate code usingzerocopyv0.8.x rather than v0.6.x. With the addition of theTryFromBytestrait, the newzerocopyrelease now has support for data-bearing enums (with some limitations due to padding bytes). Therefore, we can now supportzerocopyas an encoding forComplexerror types provided they satisfy the requirements to deriveIntoBytesandTryFromBytes. This branch changesencoding: ZerocopyIPC responses to support theComplexerror type provided that the error derives the appropriate error traits, rather than failing.Fixes #58