mgmt-gateway: Add update status and update IDs#789
Merged
Conversation
mkeeter
reviewed
Sep 19, 2022
Comment on lines
127
to
133
Collaborator
There was a problem hiding this comment.
This may be a little clearer as a basic conditional with the matches! macro, i.e.
Suggested change
| match self.update_buf.in_progress_update_id() { | |
| Some(in_progress_id) if id != in_progress_id => { | |
| return Err(ResponseError::UpdateInProgress( | |
| self.update_buf.status(), | |
| )); | |
| } | |
| _ => (), | |
| } | |
| if matches!(self.update_buf.in_progress_update_id(), | |
| Some(in_progress_id) if id != in_progress_id) | |
| { | |
| return Err(ResponseError::UpdateInProgress( | |
| self.update_buf.status(), | |
| )); | |
| } |
Contributor
Author
There was a problem hiding this comment.
I can't bring myself to like matches!. How about
if let Some(in_progress_id) = self.update_buf.in_progress_update_id() {
if id != in_progress_id {
return Err(ResponseError::UpdateInProgress(
self.update_buf.status(),
));
}
}
mkeeter
reviewed
Sep 19, 2022
Comment on lines
136
to
143
Collaborator
There was a problem hiding this comment.
Maybe replace with
Suggested change
| match self.update_task.abort_update() { | |
| // Aborting an update that hasn't started yet is fine; either way | |
| // our caller is clear to start a new update. | |
| Ok(()) | Err(UpdateError::UpdateNotStarted) => (), | |
| Err(other) => { | |
| return Err(ResponseError::UpdateFailed(other as u32)) | |
| } | |
| } | |
| // Aborting an update that hasn't started yet is fine; either way | |
| // our caller is clear to start a new update. | |
| if let Err(e) = self.update_task.abort_update() | |
| && !matches!(e, UpdateError::UpdateNotStarted) | |
| { | |
| return Err(ResponseError::UpdateFailed(e as u32)) | |
| } |
(you could avoid the awkward !matches! if ResponseError derived Eq, but that's in a separate crate)
Contributor
Author
There was a problem hiding this comment.
I'm not sure that syntax should work since let_chains are still unstable (?!). What if I rolled the rest of the method into the successful branch:
match self.update_task.abort_update() {
// Aborting an update that hasn't started yet is fine; either way
// our caller is clear to start a new update.
Ok(()) | Err(UpdateError::UpdateNotStarted) => {
self.update_buf.abort();
Ok(())
}
Err(other) => {
Err(ResponseError::UpdateFailed(other as u32))
}
}
mkeeter
reviewed
Sep 19, 2022
Collaborator
There was a problem hiding this comment.
Same suggestion as above (using a conditional + matches! instead of a match)
mkeeter
reviewed
Sep 19, 2022
Comment on lines
689
to
694
Collaborator
There was a problem hiding this comment.
Weakly-held opinion: since this function returns a Result, early-exiting may be cleaner than map, e.g.
Suggested change
| sector_erase.progress().map(|progress| { | |
| UpdateStatus::Preparing(UpdatePreparationStatus { | |
| id: sub_status.id, | |
| progress: Some(progress), | |
| }) | |
| }) | |
| let progress = sector_erase.progress()?; | |
| UpdateStatus::Preparing(UpdatePreparationStatus { | |
| id: sub_status.id, | |
| progress: Some(progress), | |
| }) |
Contributor
Author
There was a problem hiding this comment.
Yes, that's much better, thanks.
mkeeter
approved these changes
Sep 19, 2022
mkeeter
left a comment
Collaborator
There was a problem hiding this comment.
A few small suggestions for readability, but generally looks good.
This is the companion to the larger MGS change in oxidecomputer/omicron#1714. From the SP's side, the changes are relatively minor: 1. We removed "update prepare status" in favor of the more general "update status" 2. Update messages now always include an update ID, and for "update abort" we only abort if the ID matches our current in-progress update.
e988fea to
bf1b444
Compare
FawazTirmizi
pushed a commit
to rivosinc/hubris
that referenced
this pull request
Oct 3, 2022
* mgmt-gateway: add update status and IDs This is the companion to the larger MGS change in oxidecomputer/omicron#1714. From the SP's side, the changes are relatively minor: 1. We removed "update prepare status" in favor of the more general "update status" 2. Update messages now always include an update ID, and for "update abort" we only abort if the ID matches our current in-progress update.
FawazTirmizi
pushed a commit
to rivosinc/hubris
that referenced
this pull request
Oct 3, 2022
* mgmt-gateway: add update status and IDs This is the companion to the larger MGS change in oxidecomputer/omicron#1714. From the SP's side, the changes are relatively minor: 1. We removed "update prepare status" in favor of the more general "update status" 2. Update messages now always include an update ID, and for "update abort" we only abort if the ID matches our current in-progress update.
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.
This is the companion to the larger MGS change in oxidecomputer/omicron#1714. From the SP's side,
the changes are relatively minor: