pldm-fw: ua: send actual component count in RequestUpdate#59
Open
brcarr-nv wants to merge 1 commit into
Open
Conversation
request_update() hardcoded the RequestUpdate NumberOfComponents field to 1. DSP0267 Table 27 defines this field as "the number of components that will be passed to the FD during the update", and the FD may use it to compare against the number of PassComponentTable/UpdateComponent commands it receives. When a package applies more than one component, the UA passes all of them (pass_component_table and update_components_progress iterate over update.components) while still announcing only 1, which a conformant FD can reject as a mismatch. Derive the count from update.components, the same list driven through the rest of the update flow, so the announced value matches what is actually sent. The value is fallibly converted to u16 to guard the (practically impossible) >65535 component case rather than truncating. Add a unit test (request_update_reports_actual_component_count) that drives request_update over a mock MCTP ReqChannel and asserts the RequestUpdate NumberOfComponents field reflects the actual component count, along with the cfg(test) package-builder and temp-file helpers it depends on. Signed-off-by: Brian Carr <brcarr@nvidia.com>
mkj
requested changes
Jul 20, 2026
mkj
left a comment
mkj
left a comment
Member
There was a problem hiding this comment.
Thanks for the fix. Some comments on simplifying tests
Comment on lines
+349
to
+350
| #[cfg(test)] | ||
| pub(crate) fn build_v11_package(vid: u16, components: &[&[u8]]) -> Vec<u8> { |
Member
There was a problem hiding this comment.
This can go in a
#[cfg(test)]
pub(crate) mod tests {|
|
||
| #[derive(Clone, Default)] | ||
| struct MockComm { | ||
| st: Rc<RefCell<CommState>>, |
Member
There was a problem hiding this comment.
No need for Rc<RefCell>, send_vectored() and recv() take &mut self.
So responses/send can be members of MockComm, don't need CommState
| /// The file is unlinked immediately; the returned handle keeps it alive, so | ||
| /// the on-disk entry is cleaned up automatically once the handle is dropped. | ||
| #[cfg(test)] | ||
| pub(crate) fn temp_file_with(bytes: &[u8]) -> std::fs::File { |
Member
There was a problem hiding this comment.
For tests I'd be fine with a dev-dependency of tempfile::tempfile(), but this looks right too.
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.
request_update() hardcoded the RequestUpdate NumberOfComponents field to 1. DSP0267 Table 27 defines this field as "the number of components that will be passed to the FD during the update", and the FD may use it to compare against the number of PassComponentTable/UpdateComponent commands it receives. When a package applies more than one component, the UA passes all of them (pass_component_table and update_components_progress iterate over update.components) while still announcing only 1, which a conformant FD can reject as a mismatch.
Derive the count from update.components, the same list driven through the rest of the update flow, so the announced value matches what is actually sent. The value is fallibly converted to u16 to guard the (practically impossible) >65535 component case rather than truncating.