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

Cleanup usage of cfg(target_arch = "...") in doc tests #293

Merged
merged 8 commits into from
Nov 4, 2024

Conversation

roypat
Copy link
Collaborator

@roypat roypat commented Oct 31, 2024

Summary of the PR

This is a follow up to #208 (it came up during review, but was definitely out of scope there):

  • Hide cfg(target_arch = "...") from cargo doc output, as it's clutter, and the indentation it comes with is somewhat unsightly imo
  • Remove cfgs if they're not needed in the first place (either because the function the doctest is attached to is already cfg'd, or because the doctest works on all supported architectures)
  • Refactor some doc test to work on more architectures (guest_memfd related doctests mostly, in anticipation that it'll be supported on ARM eventually)

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.

@roypat roypat force-pushed the doctest-cleanup branch 2 times, most recently from 9f17761 to d6b50ba Compare October 31, 2024 11:45
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
- Convert `assert!(... == ...)` to `assert_eq!`
- Drop unneeded `any` inside cfgs if the `any` only contains a single
  condition.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
- Remove unneeded cfg(target_arch = ...) from doc tests of functions
  that are already cfg'd themselves to only exist on that arch
- Remove unneeded code block in cpuid doc test.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
When they are needed, hide them from carg doc output (in favor of a
comment above the example that calls out which architectures the example
will work on). If they're not needed (e.g. because the function itself
is already cfg'd to that specific architecture), drop them. In some
cases, replace cfgs with explicit checks of KVM capabilities
(specifically around guest_memfd stuff).

Also fix up the guest_memfd doctests not using a VM type that supports
guest private memory (if one exists, aka only on x86).

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Add KVM_CAP_{GUEST_MEMFD, USER_MEMORY2, MEMORY_ATTRIBUTES} to the `Cap`
enum, so they can be used with `VmFd::check_extension`.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat marked this pull request as ready for review October 31, 2024 11:52
The KVM docs do not list this ioctls as restricted to any specific
architecture. It is instead discoverable via capabilities, and can
theoretically work on any architecture (provided the architecture has a
VM type that support guest private memory).

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
@roypat roypat requested a review from rbradford October 31, 2024 13:06
@ShadowCurse ShadowCurse merged commit d7cfcad into rust-vmm:main Nov 4, 2024
23 checks passed
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