-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change IoVecBuffer[Mut] len to u32 #4556
Conversation
ad3f4c2
to
d829d68
Compare
Fixing lint/build errors... |
9e7f47b
to
2c51bdd
Compare
Local Tests on R7g.metal:Checkstyle
Integration tests
Performance Tests
Performance tests fail on R7g because of the graviton 3 processors having a different cpu register set? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4556 +/- ##
==========================================
- Coverage 82.08% 82.08% -0.01%
==========================================
Files 255 255
Lines 31256 31258 +2
==========================================
+ Hits 25658 25659 +1
- Misses 5598 5599 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @brandonpike,
Thanks for picking this up! The changes generally look good, but I think we can go a bit further in the vsock module. The specification states that everything size related about vsock buffers is u32, so we can do the following changes to function signatures:
VsockConnection::peer_avail_credit
can returnu32
VsockPacket::buf_size
can returnu32
VsockPacket::read_at_offset_from
should be(&self, &mut T, u32, u32) -> Result<u32, VsockError>
(e..g. the conversion to usize should only happen right around thewrite_volatile_at
call- ditto for
VsockPacket::write_from_offset_to
Although, maybe we can even get away with changing the argumentsIoVecBuffer[Mut]::{read,write}_volatile_at
tou32
s?
That being said, the changes in this PR as-is are already a huge improvement, so am also happy to approve/merge and track the above as a follow up. Just please fix the errors in the cfg(kani)
modules :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now! As one last thing, could you squash the fixup commit into 2c51bdd? Then we should be good to merge :)
This commit changes the iovec len primitive to match descriptor chain's (u32). This removes some ugly casting and potential overflow problems, and allows us to upcast when needed in a non-lossy manor. Signed-off-by: Brandon Pike <bpike@amazon.com>
8a628ee
to
feae33f
Compare
Changes
Closes #4548
Reason
Protect against overflows and use a consistent data type for virtio files.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md
.PR Checklist
PR.
CHANGELOG.md
.TODO
s link to an issue.contribution quality standards.
rust-vmm
.