virtio-queue: add DescriptorChainReader and Writer

[aesteve-rh]

Summary of the PR

The existing Reader<'a, B> and Writer<'a, B> borrow guest memory with an external lifetime parameter, which prevents them from being used as concrete type parameters in structs that need to be stored across requests. Currently, vhost-device-media workarounds it by duplicating the I/O logic locally.

Add DescriptorChainReader and DescriptorChainWriter, which own the guest memory accessor through the DescriptorChain they consume rather than borrowing it from an external reference. Both implement std::io::Read and std::io::Write respectively, and expose bytes_read() and bytes_written() counters. Because they carry no lifetime parameter, they can be stored directly as fields or type parameters in device handler structs.

Unlike the existing types, these are lazy: descriptors are traversed one at a time per read/write call rather than pre-fetched at construction.

To enable this without requiring M: Clone on the reader/writer constructors (as happens with vhost-device-media implementation), add a memory() accessor to DescriptorChainRwIter, which gives access to the guest memory held inside the iterator's DescriptorChain without the caller needing to retain a separate handle.

Used an agent for generating tests, docs, and reviews.

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.

[aesteve-rh]

Comes from the discussion at rust-vmm/vhost-device#944 (comment)

[aesteve-rh]

Tests were generated by an agent as specified in the PR/commit body, but I have also tested locally with vhost-device-media (replacing the types in descriptor_chain.rs by the one added here) and confirmed it to work.

[epilys]

I haven't used this new API in detail, but wouldn't it be possible to store owned descriptor chains somewhere and create the already existing borrowed reader/writers on demand whenever we need to use them?

[epilys]

Or, the API could take a lifetime, like std::borrow::Cow and use 'static when it's owned.

[aesteve-rh]

I haven't used this new API in detail, but wouldn't it be possible to store owned descriptor chains somewhere and create the already existing borrowed reader/writers on demand whenever we need to use them?

But Reader<'a, B> still requires the 'a lifetime tied to something. The lifetime is tied to the borrow of chain, so it ends up being self-referencial struct. It won't work without using unsafe code or raw pointers.

Or, the API could take a lifetime, like std::borrow::Cow and use 'static when it's owned.

Cow<'a, B> just moves the problem up. Every struct that stores it will need to be parametrized with 'a. To avoid it (which is what we intend here), it requires a 'static bound as you said, which is too restrictive for guest memory (it needs to be remappable)?

[epilys]

Thanks for the answers.

Just a note:

which is too restrictive for guest memory

A Cow<'static, _> does not mean the data is 'static, since it derefs to the lifetime of the Cow. The lifetime 'a in Cow<'a, _> refers to the Borrowed variant's lifetime.

use std::ops::Deref;
use std::borrow::Cow;

fn take_static(s: &'static str) {
    println!("s = {s}");
}

fn main() {
    let a: Cow<'static, str> = Cow::Borrowed("a");
    let b: Cow<'static, str> = Cow::Owned("b".to_string());
    
    take_static(a.deref());
    take_static(b.deref());
}

error[E0597]: `a` does not live long enough
  --> src/main.rs:12:17
   |
 9 |     let a: Cow<'static, str> = Cow::Borrowed("a");
   |         - binding `a` declared here
...
12 |     take_static(a.deref());
   |     ------------^---------
   |     |           |
   |     |           borrowed value does not live long enough
   |     argument requires that `a` is borrowed for `'static`
13 |     take_static(b.deref());
14 | }
   | - `a` dropped here while still borrowed

error[E0597]: `b` does not live long enough
  --> src/main.rs:13:17
   |
10 |     let b: Cow<'static, str> = Cow::Owned("b".to_string());
   |         - binding `b` declared here
...
13 |     take_static(b.deref());
   |     ------------^---------
   |     |           |
   |     |           borrowed value does not live long enough
   |     argument requires that `b` is borrowed for `'static`
14 | }
   | - `b` dropped here while still borrowed

So the analogous reader enum would be sth like:

pub enum Reader<'a, B> {
  Borrowed(Reader<'a, B>),
  Owned(DescriptorChainReader<M>),
}

But this is unrelated to this PR, so nevermind :)

[aesteve-rh]

Oh. That's right. Thanks for the clarification and examples.

Following your example we could do something like:

pub enum Reader<'a, B = (), M = ()> {
    Borrowed(BorrowedReader<'a, B>),  // current Reader, renamed
    Owned(DescriptorChainReader<M>),
}

pub type OwnedReader<M> = Reader<'static, (), M>;

// And analogous
pub type OwnedWriter<M> = Writer<'static, (), M>;

Changing Reader into an enum is a breaking change, maybe we could play a bit with the names to try to avoid it. But either way, it would also need a io::Read implementation for both borrowed (original) and owned variants, which is probably non trivial with () type.

I can try to explore it further if you think it'll make for a better design in the long term.

[epilys]

I can try to explore it further if you think it'll make for a better design in the long term.

No need, I was just thinking out loud. Will review your PR separately.

[stefano-garzarella]

Changing Reader into an enum is a breaking change, maybe we could play a bit with the names to try to avoid it.

We are still at 0.y so we can break if it makes the API more usable IMO. Also this is not widely used IIRC, so maybe just some adjustments in vhost-device crates should be enough.

[aesteve-rh]

If we can afford this breaking change maybe we could rename Reader/Writer to BorrowedReader/BorrowedWriter, and then use OwnedReader/OwnedWriter for the new types.

But I'm not sure we should put effort into the single enum pattern though. The discussion was interesting, but it will add more types and complexity that what I'm doing here.