Skip to content
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

aarch64: Reorganize test_create_device #281

Merged
merged 3 commits into from
Sep 30, 2024

Conversation

TimePrinciple
Copy link
Contributor

Summary of the PR

Add has_device_attr check in both set_supported_nr_irqs and request_gic_init, effectively enabling reuse of request_gic_init in test_create_device.

I was going through these tests while introducing RISC-V ioctls. Some minor improvements :)

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

src/ioctls/device.rs Outdated Show resolved Hide resolved
src/ioctls/vm.rs Outdated Show resolved Hide resolved
Add `has_device_attr` check in both `set_supported_nr_irqs` and
`request_gic_init`, effectively enabling reuse of `request_gic_init` in
`test_create_device`.

Signed-off-by: Ruoqing He <[email protected]>
As @roypat pointed out, and quote:

"Asserting on .is_ok()/.is_err() leads to hard to debug failures (as if
the test fails, it will only say "assertion failed: false". We replace
these with `.unwrap()`, which also prints the exact error variant that
was unexpectedly encountered (we can to this these days thanks to
efforts to implement Display and Debug for our error types). If the
assert!((...).is_ok()) was followed by an .unwrap() anyway, we just drop
the assert."

Signed-off-by: Ruoqing He <[email protected]>
.cargo/config.toml Outdated Show resolved Hide resolved
As @roypat pointed out, and quote:

"This lint disallows asserttions on is_ok()/is_err() in favor of either
using unwrap (so that at least if the test fails, we the failure message
will contain the actual failure reason instead of just "was not
ok/err"), or actually matching the specific variant."

Signed-off-by: Ruoqing He <[email protected]>
Copy link
Contributor

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Great improvements. I especially like the assertion cleanups.

@TimePrinciple
Copy link
Contributor Author

Great improvements. I especially like the assertion cleanups.

If you like, I will do it in cloud-hypervisor :)

@roypat roypat merged commit 488a2d3 into rust-vmm:main Sep 30, 2024
19 checks passed
@TimePrinciple TimePrinciple deleted the repurpose-arm-device-tests branch September 30, 2024 17:40
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.

3 participants