Don't return None from BlockData::block_for_req when attachment is paused#711
Merged
Conversation
Contributor
Author
|
The new Clippy failures seem to be Rust 1.79-related--I only started seeing them locally after a The failed PHD run seems to be due to an error downloading an artifact from Buildomat: I've submitted this for a rerun. |
pfmooney
reviewed
Jun 14, 2024
pfmooney
left a comment
Contributor
There was a problem hiding this comment.
Thanks for fixing this.
Comment on lines
73
to
75
| match check_state(att_state) { | ||
| Err(ReqError::Stopped | ReqError::Detached) => { | ||
| return None; | ||
| } | ||
| Ok(()) | ||
| | Err(ReqError::Paused | ReqError::NonePending) => { | ||
| let _guard = self.cv.wait(guard).unwrap(); | ||
| continue; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
I would just make the calls to is_attached() and is_stopped() here, instead of using check_state. Handling the NonePending error case is misleading, since check_state never emits it. It suggest that more is being checked than really is.
Contributor
Author
There was a problem hiding this comment.
Good call, I'll fix this up.
8fb8f43 to
099e2be
Compare
pfmooney
approved these changes
Jun 14, 2024
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Change
BlockData::block_for_reqso that it doesn't return early when the device/backend pair is paused. (The async block backend's request future doesn't have this issue.)Add PHD tests for guest reboot (both in-guest and API-driven) to verify the fix; the non-Crucible variants of these tests fail without this fix and pass with them. The Crucible variants (which use the async driver) pass without issue, but it's useful to get coverage there, too.
Fix PHD's raw-buffering serial adapter so that, if a caller asks to wait for a string to appear in the buffer, the buffer properly trims its contents when the wait is satisfied. This is needed to keep the new tests from passing trivially; without the fix, the first login sequence to appear on the serial console satisfies all of the "wait for guest to boot" calls in the rest of the test.
Tests: PHD runs of Alpine and Debian 11 w/new test cases.
Fixes #709. Fixes #710.