Skip to content

Enforce clippy integration test on aarch64 #1961

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

Merged
merged 4 commits into from
Jul 1, 2020

Conversation

dianpopa
Copy link
Contributor

@dianpopa dianpopa commented Jun 16, 2020

Reason for This PR

Fixes #1007.

Description of Changes

Fixes clippy errors and enforce check in test_style.

  • This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes are reflected in firecracker/swagger.yaml.
  • Any user-facing changes are mentioned in CHANGELOG.md.

@dianpopa dianpopa self-assigned this Jun 16, 2020
@dianpopa dianpopa requested a review from a team June 16, 2020 17:16
@dianpopa dianpopa requested a review from a team June 17, 2020 13:16
Comment on lines 315 to 314
if num_cpus > 1 {
if cpu_index > 0 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here seems different than it was before. For example, if num_cpus was 2, we were appending the property for every cpu, now we won't append it for the first one.

Also, would be nice to add a comment or a pointer here why we do this check, I have troubles finding it in the aforementioned doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, @lauralt ! I updated the code and also added a more explicit comment. Take a look!

@dianpopa dianpopa force-pushed the little_fixes_8 branch 2 times, most recently from 28af370 to 09e5a9b Compare July 1, 2020 10:45

for cpu_index in 0..num_cpus {
let more_than_one = vcpu_mpidr.len() > 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more_tan_one sounds a bit confusing to me. I would rename it to has_multiple_vcpus or I would keep the old variable num_cpus = vcpu_mpidr.len();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Take a look!

dianpopa added 2 commits July 1, 2020 11:35
Signed-off-by: Diana Popa <dpopa@amazon.com>
Also, adjusted code to also run clippy on gnu code (mostly found
in the seccomp logic).

Signed-off-by: Diana Popa <dpopa@amazon.com>
@lauralt lauralt merged commit f4defb3 into firecracker-microvm:master Jul 1, 2020
@dianpopa dianpopa deleted the little_fixes_8 branch July 1, 2020 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add clippy integration test for aarch64
3 participants