firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Change IoVecBuffer[Mut] len to u32

Open brandonpike opened this issue 1 year ago • 3 comments

Changes

  • Change IoVecBuffer[Mut] len to u32 (was usize)
  • Add u32 overflow check (errors out rather than panics)

Closes https://github.com/firecracker-microvm/firecracker/issues/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

  • [x] If a specific issue led to this PR, this PR closes the issue.
  • [x] The description of changes is clear and encompassing.
  • [x] Any required documentation changes (code and docs) are included in this PR.
  • [ ] API changes follow the Runbook for Firecracker API changes.
  • [x] User-facing changes are mentioned in CHANGELOG.md.
  • [x] All added/changed functionality is tested.
  • [x] New TODOs link to an issue.
  • [x] Commits meet contribution quality standards.

  • [x] This functionality cannot be added in rust-vmm.

brandonpike avatar Apr 12 '24 16:04 brandonpike

Fixing lint/build errors...

brandonpike avatar Apr 24 '24 19:04 brandonpike

Local Tests on R7g.metal:

Checkstyle

./tools/devtool -y checkstyle
======================================================================= 13 passed in 22.86s =======================================================================
[Firecracker devtool 2024-04-24T22:10:26+00:00] Finished test run ...

Integration tests

./tools/devtool -y test -- ../tests/integration_tests/build/
======================================================= 8 passed, 1 skipped, 1 warning in 384.33s (0:06:24) =======================================================
[Firecracker devtool 2024-04-24T22:08:11+00:00] Finished test run ...

Warning:
/firecracker/tests/integration_tests/build/test_coverage.py:106: UserWarning: Not uploading coverage report due to missing CODECOV_TOKEN environment variable
    warnings.warn(

Performance Tests

./tools/devtool -y test --performance -c 1-10 -m 0 -- ../tests/integration_tests/ -m 'no_block_pr and not nonci' --log-cli-level=INFO
FAILED

Performance tests fail on R7g because of the graviton 3 processors having a different cpu register set?

brandonpike avatar Apr 24 '24 22:04 brandonpike

Codecov Report

Attention: Patch coverage is 96.29630% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 82.08%. Comparing base (0f39350) to head (ba36b81).

Files Patch % Lines
src/vmm/src/devices/virtio/vsock/mod.rs 0.00% 1 Missing :warning:
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     
Flag Coverage Δ
4.14-c5n.metal 79.58% <96.29%> (-0.01%) :arrow_down:
4.14-c7g.metal ?
4.14-m5n.metal 79.56% <96.29%> (-0.01%) :arrow_down:
4.14-m6a.metal 78.79% <96.29%> (-0.01%) :arrow_down:
4.14-m6g.metal 76.62% <96.29%> (-0.01%) :arrow_down:
4.14-m6i.metal 79.55% <96.29%> (-0.01%) :arrow_down:
4.14-m7g.metal 76.62% <96.29%> (-0.01%) :arrow_down:
5.10-c5n.metal 82.09% <96.29%> (-0.01%) :arrow_down:
5.10-c7g.metal ?
5.10-m5n.metal 82.08% <96.29%> (-0.01%) :arrow_down:
5.10-m6a.metal 81.39% <96.29%> (+<0.01%) :arrow_up:
5.10-m6g.metal 79.40% <96.29%> (-0.01%) :arrow_down:
5.10-m6i.metal 82.07% <96.29%> (-0.01%) :arrow_down:
5.10-m7g.metal 79.40% <96.29%> (-0.01%) :arrow_down:
6.1-c5n.metal 82.09% <96.29%> (-0.01%) :arrow_down:
6.1-c7g.metal ?
6.1-m5n.metal 82.08% <96.29%> (-0.01%) :arrow_down:
6.1-m6a.metal 81.38% <96.29%> (-0.01%) :arrow_down:
6.1-m6g.metal 79.40% <96.29%> (-0.01%) :arrow_down:
6.1-m6i.metal 82.07% <96.29%> (-0.01%) :arrow_down:
6.1-m7g.metal 79.40% <96.29%> (-0.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 25 '24 07:04 codecov[bot]