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

io: make Read/WriteVolatile implementations optional #311

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

bonzini
Copy link
Member

@bonzini bonzini commented Jan 28, 2025

Summary of the PR

Some projects may not want I/O to go directly to libc, for example to avoid any kind of blocking I/O. Make the file descriptor implementations of ReadVolatile and WriteVolatile optional.

The PR is on top of #310.

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.

@bonzini bonzini changed the title io: allow compiling without the libc dependency io: make Read/WriteVolatile implementations optional Jan 28, 2025
@bonzini bonzini marked this pull request as ready for review January 28, 2025 12:19
CHANGELOG.md Outdated Show resolved Hide resolved
@bonzini bonzini force-pushed the optional-libc branch 4 times, most recently from 995c720 to d39af30 Compare January 28, 2025 13:32
roypat
roypat previously approved these changes Jan 28, 2025
@roypat
Copy link
Collaborator

roypat commented Jan 28, 2025

Btw Paolo, while I got your attention here in rust-vmm land, any chance you have a second to look at #307? :o

@stefano-garzarella
Copy link
Member

@bonzini can you check tests when only backend-mmap or xen is enabled?

I had something like that (with xen looks similar):

$ cargo test --no-default-features --features=backend-mmap
   Compiling vm-memory v0.16.1 (/home/stefano/repos/vm-memory)
error[E0277]: the trait bound `std::fs::File: io::ReadVolatile` is not satisfied
    --> src/mmap.rs:1165:47
     |
1165 |             gm.read_exact_volatile_from(addr, &mut file, mem::size_of::<u32>())
     |                ------------------------       ^^^^^^^^^ the trait `io::ReadVolatile` is not implemented for `std::fs::File`
     |                |
     |                required by a bound introduced by this call
     |
     = help: the following other types implement trait `io::ReadVolatile`:
               &[u8]
               std::io::Cursor<T>
note: required by a bound in `guest_memory::GuestMemory::read_exact_volatile_from`
    --> src/guest_memory.rs:739:12
     |
732  |     fn read_exact_volatile_from<F>(
     |        ------------------------ required by a bound in this associated function
...
739  |         F: ReadVolatile,
     |            ^^^^^^^^^^^^ required by this bound in `GuestMemory::read_exact_volatile_from`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `vm-memory` (lib test) due to 1 previous error

This reminds me that we should work on rust-vmm/rust-vmm-ci#152
I'll try to allocate some time after FOSDEM.

@bonzini bonzini force-pushed the optional-libc branch 2 times, most recently from b08e497 to a132296 Compare January 29, 2025 18:12
@bonzini
Copy link
Member Author

bonzini commented Jan 29, 2025

All combinations (tested with cargo hack --feature-powerset build --tests, while test --all is in progress) work now.

@bonzini bonzini requested a review from roypat January 29, 2025 18:14
@stefano-garzarella
Copy link
Member

All combinations (tested with cargo hack --feature-powerset build --tests, while test --all is in progress) work now.

Thanks for checking!

@stefano-garzarella
Copy link
Member

@bonzini I think there is another failure in some examples which are tested only when running the tests (so maybe you already discovered running the all tests, but I'm reporting in any case):

$ git describe              
v0.11.0-101-ga132296

$ cargo test --no-default-features --features backend-mmap
...
failures:

---- src/guest_memory.rs - guest_memory::GuestMemory::read_volatile_from (line 652) stdout ----
error[E0277]: the trait bound `File: ReadVolatile` is not satisfied
   --> src/guest_memory.rs:672:29
    |
23  | gm.read_volatile_from(addr, &mut file, 128)
    |    ------------------       ^^^^^^^^^ the trait `ReadVolatile` is not implemented for `File`
    |    |
    |    required by a bound introduced by this call
    |
    = help: the following other types implement trait `ReadVolatile`:
              &[u8]
              std::io::Cursor<T>
note: required by a bound in `read_volatile_from`
   --> /home/stefano/repos/vm-memory/src/guest_memory.rs:682:12
    |
680 |     fn read_volatile_from<F>(&self, addr: GuestAddress, src: &mut F, count: usize) -> ...
    |        ------------------ required by a bound in this associated function
681 |     where
682 |         F: ReadVolatile,
    |            ^^^^^^^^^^^^ required by this bound in `GuestMemory::read_volatile_from`

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0277`.
Couldn't compile the test.

failures:
    src/guest_memory.rs - guest_memory::GuestMemory::read_volatile_from (line 652)

test result: FAILED. 42 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.97s

@bonzini
Copy link
Member Author

bonzini commented Jan 30, 2025

I forgot to push that one, sorry.

@roypat roypat merged commit 0b7a437 into rust-vmm:main Jan 30, 2025
2 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