virtio_queue: Add descriptors_utils.rs

[MatiasVara]

Summary of the PR

This commit adds descriptors_utils.rs from virtiofsd (daa4e32). This module provides the Reader/Writer traits to iterate over descriptors. Devices shall rely on this interface instead of manipulate descriptors. The reason to this is that the distribution is driver-dependent, i.e., virtio does not force any particular way to distribute the data over descriptors. The version in this commit removes read_to_at() and write_from_at() functions that are required only for virtiofsd. This commit is on top of 0.9 tag. To try this module, I am adding support for it in the virtio-sound device at https://github.com/MatiasVara/vhost-device/tree/use-descriptor-utils-virtio-sound.

Requirements

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

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

[MatiasVara]

@germag feel free to comment.

[Ablu]

@MatiasVara thanks a lot for updating this! This is something which we really need. I will review this tomorrow in more detail!

Just a couple of minor comments before diving into a review:

• Would be great to add a reference from which virtiofsd version this was adapted
• Has someone done a comparison whether there have been new changes on this over on the crosvm project?

[MatiasVara]

@MatiasVara thanks a lot for updating this! This is something which we really need. I will review this tomorrow in more detail!

Just a couple of minor comments before diving into a review:

• Would be great to add a reference from which virtiofsd version this was adapted
• Has someone done a comparison whether there have been new changes on this over on the crosvm project?

I missed those two points from our previous discussion. I will add a reference to the virtiofsd version. In regards to the second point, I just did a quick review of the crosvm version and found that it doesn't require a lifetime parameter when instantiated. However, I did not go any further. The differences may need to be better understood.

[stsquad]

Is virtiofsd's read/write_at not likely to be useful for other backends? Is it a very special case only virtiofsd benefits from?

[MatiasVara]

Is virtiofsd's read/write_at not likely to be useful for other backends? Is it a very special case only virtiofsd benefits from?

Probably, I can add them if they are required for other backends. If not, I believe each device can "adapt or extend" current traits to suit its particular needs.

[MatiasVara]

@Ablu Thanks for the review! I will try to answer some of the comments during today.

Some random comments, but nothing extraordinary. One thing I wonder about is whether the separation into Reader and Writer makes a lot of sense for the user.

Does a user of this really care that much about it? I wonder whether it could not be a single object and the API could be used something like:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

(maybe overly simplified, but should show the idea)

With the proposed API here that has to be done manually like writter_status.split_at(size_of::<VirtioSoundHeader>()). While this certainly bring great improvements for the user, I wonder whether the ergonomics could be even better with a single object? 🤔 Sure, some special case may require "seeking" in the chain, but I guess 90%+ of the uses will just be sequential processing/parsing followed by writing the answer?

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

[Ablu]

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

Ah. Did not realize that readable first, writeable after that even was a global rule. Not sure whether we need specific logic to verify this (they flags on the descriptor sound sufficient to me). But it confirms that read, then write is the only case :).

@MatiasVara: Want to try to adapt it to a single object? Eventually, I think we should target for this API being the default way to work with descriptors. So probably the descriptor chain iterator on the queue should in the end just return something that implements this high-level API :).

[MatiasVara]

Having a single object would simplify things. My guess about this design is that the chain of descriptors could be divided into two areas: a chain with read-only descriptors followed by a chain with write-only descriptors. This is the only requirement from the virtio-spec about the descriptors distribution (2.6.4.2 in Virtio Spec v1.1). As far as I understand it, when a Reader and Writer are instantiated, they "point" to the beginning of the area they correspond to. A single object shall automatically take into account such an offset. This is not a problem at all, and such a logic should be added to write/read_obj().

Ah. Did not realize that readable first, writeable after that even was a global rule. Not sure whether we need specific logic to verify this (they flags on the descriptor sound sufficient to me). But it confirms that read, then write is the only case :).

@MatiasVara: Want to try to adapt it to a single object? Eventually, I think we should target for this API being the default way to work with descriptors. So probably the descriptor chain iterator on the queue should in the end just return something that implements this high-level API :).

Yes, I can try to adapt it to a single object.

[MatiasVara]

Some random comments, but nothing extraordinary. One thing I wonder about is whether the separation into Reader and Writer makes a lot of sense for the user.

Does a user of this really care that much about it? I wonder whether it could not be a single object and the API could be used something like:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

(maybe overly simplified, but should show the idea)

With the proposed API here that has to be done manually like writter_status.split_at(size_of::<VirtioSoundHeader>()). While this certainly bring great improvements for the user, I wonder whether the ergonomics could be even better with a single object? 🤔 Sure, some special case may require "seeking" in the chain, but I guess 90%+ of the uses will just be sequential processing/parsing followed by writing the answer?

Would it make sense an interface like this? Note that this is just a draft and names will be different:

pub struct Walker<'a>{
    r: Reader<'a>,
    w: Writer<'a>,
    payload: Writer<'a>
}

impl <'a> Walker<'a>{
    pub fn new<M>(
        mem: &'a GuestMemoryMmap,
        desc_chain: DescriptorChain<M>,
        offset: usize
    ) -> Result<Walker<'a>>
    where
       M: Deref,
       M: Clone,
       M::Target: GuestMemory + Sized,
    {
        let reader = Reader::new(mem, desc_chain.clone()).unwrap();
        let mut writer = Writer::new(mem, desc_chain.clone()).unwrap();
        let payload = writer.split_at(offset).unwrap();
        Ok(Walker {
            r: reader,
            w: writer,
            payload
        })
    }

    pub fn read_obj<T: ByteValued>(&mut self) -> io::Result<T> {
        self.r.read_obj()
    }

    pub fn write_obj<T: ByteValued>(&mut self, val: T) -> io::Result<()> {
        self.w.write_obj(val)
    }

    pub fn write_payload_obj<T: ByteValued>(&mut self, val: T) -> io::Result<()> {
        self.payload.write_obj(val)
    }
}

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

[stefano-garzarella]

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

[MatiasVara]

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

[Ablu]

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

[MatiasVara]

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

[stefano-garzarella]

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status). What about having a vector of Writers/Readers, then using an index?

[Ablu]

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

Sorry for not being super clear... What I meant was I see no pressing need for having Writer and Reader types for walking the descriptor chain.

Going back to what I sketched before:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

Here, descriptor_chain could directly be what this PR currently calls DescriptorChainConsumer (probably merged into or containing the current desc_chain type). Then read_obj()/write_obj could be defined on that while keeping some internal cursor.

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

:thinking:

[MatiasVara]

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

I do not think payload is required for all the devices. In those cases, I would set offset = 0. WDYT?

What is the payload use case?

I used to use the split_at() function on the writer to get a new writer that begins in the offset in which the payload of the request starts. In the case of virtio-sound, on write-only descriptors, there is first the status and then the payload but we first write the payload and then the status. I would require a sort of seek() to first write the payload and then the status (or just change the way we currently do it). Since all requests have a status, I proposed to split_at when Walker is instantiated and then provide a method to explicitly write in the area that corresponds with the payload which is after the status header.

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status). What about having a vector of Writers/Readers, then using an index?

Could you elaborate? I proposed payload to work around split_at() which is not simply to implement if both Reader and Writer are in the same object. I think split_at() is useful because it allows you to write to the write-only descriptor chain in a independent ways. For example, you can have on Writer for the payload and another for the status. Otherwise, you should require a sort of seek() before to write in the correct position.

[stefano-garzarella]

mmm, payload it looks like too specific for virtio-sound use case, for example in virtio-blk is it the opposite (payload is before the status). What about having a vector of Writers/Readers, then using an index?

Could you elaborate? I proposed payload to work around split_at() which is not simply to implement if both Reader and Writer are in the same object.

Yeah, but this is meaningful only for virtio-sound, because for example for virito-blk the payload is before the status.

For some others maybe payload doesn't mean much, so IMHO we should provide more generic API. What I was suggesting was for example provide a vector of offsets to the constructor, where to split the Writers or Readers, then use the index in the API to write a specific section we split.

I think split_at() is useful because it allows you write to the write-only descriptor chain in a independent way.

If we want a single Walker, then maybe we need to provide methods to split the Walker, but I don't think we should split into header and payload in it, because that's really device-specific and the user has to do the split.

[MatiasVara]

I wonder: Why do we need separate Reader/Writer types at all? Couldn't we just use one that implements read/write mechanism directly (a bit like Cursor from the standard lib)?

Could you elaborate?, Thanks.

Sorry for not being super clear... What I meant was I see no pressing need for having Writer and Reader types for walking the descriptor chain.

Going back to what I sketched before:

for desc_chain in requests {
    let header = desc_chain.read_obj::<SomeHeader>()?;
    let response = ...;
    desc_chain.write_obj(response)?;
}

I think you can't use the same desc_chain for reading and writing but I may be wrong. I think a single object would still have two desc_chain one for reading and another for writing. In such a case, split_at may be hard to implement.

Here, descriptor_chain could directly be what this PR currently calls DescriptorChainConsumer (probably merged into or containing the current desc_chain type). Then read_obj()/write_obj could be defined on that while keeping some internal cursor.

I am not exactly sure whether I understand why the payload differentiation is needed though. Why can't I just do:

desc_chain.read_obj::<SomeHeader>()?;
desc_chain.write_obj(payload)?;
desc_chain.write_obj(status)?;

🤔

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

[Ablu]

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

I wonder: Couldn't one just .clone() at the appropriate state in order to detach one reader/writer instance from another?

[MatiasVara]

I used payload to work around split_at(). The status is not always at the end of the chain. In that case, you would need a sort of seek before the write_obj(). I used split_at() instead so I can write the payload and the status independently.

I wonder: Couldn't one just .clone() at the appropriate state in order to detach one reader/writer instance from another?

I think the example could be rewritten as the following draft:

desc_chain_reader.read_obj::<SomeHeader>()?;
desc_chain_writer.write_obj(payload)?;
desc_chain_writer_status = desc_chain_writer.clone();
desc_chain_writer_status.seek(sizeof(payload)); // note that seek() is not currently implemented
desc_chain_writer.write_obj(status)?;

I think it is still required to separate the desc_chain for reading and for writing. I am not sure if the use of such a clone should require the use of Arc() or Mutex(). Also, seek() is not currently implemented. The same using split_at() would be (it is a draft):

reader.read_obj::<SomeHeader>()?;
writer_status = writer_content.split_at(sizeof(payload))
writer_status.write_obj(status)?;

This allows users to easily share writer_content and writer_status. For example, writer_content may be used as a parameter to a function that processes the content, etc. In the case of a single object, if seek() is not provided, a split() would result in an object that would still contain a reference to a reader/writer that is not needed any more.

I think we have to modify current implementation only if changes improve the way it is currently used. It is also true that those changes may raise after some utilization in the future. The other way to think would be to let each device add descriptor_utils.rs (as it is done for virtiofsd) and modify it as needed. In the future, we can move descriptor_utils.rs to virtio-queue crate when it is more clear how other devices use it. It is not clear to me how to modify Reader/Writer in a single object in a way that produces any benefits. I am not saying that that does not exist. A third option would be to provide something much more simpler, e.g., what we proposed for virtio-sound, in which some API simply allows reading and writing over descriptors in a continuous way so the device is allowed to support any distribution. This would require rewriting descriptor_utils from scratch, which is not the goal of this PR.    

[Ablu]

I think it is still required to separate the desc_chain for reading and for writing.

Why is that? :thinking: Chances are that I misunderstand something, but I have not found the reason spelled out so far.

I think we have to modify current implementation only if changes improve the way it is currently used. It is also true that those changes may raise after some utilization in the future. The other way to think would be to let each device add descriptor_utils.rs (as it is done for virtiofsd) and modify it as needed. In the future, we can move descriptor_utils.rs to virtio-queue crate when it is more clear how other devices use it. It is not clear to me how to modify Reader/Writer in a single object in a way that produces any benefits. I am not saying that that does not exist. A third option would be to provide something much more simpler, e.g., what we proposed for virtio-sound, in which some API simply allows reading and writing over descriptors in a continuous way so the device is allowed to support any distribution. This would require rewriting descriptor_utils from scratch, which is not the goal of this PR.

I think unless we have some upstream solution, people probably won't put the brain cycles into creating a one-fits-all solutions that suits everyone equally well. I was hoping that one could turn it into a simpler to use API with just a few tweaks. I will try to play with this somewhen during this week and see how it feels.

[MatiasVara]

I think it is still required to separate the desc_chain for reading and for writing.

Why is that? 🤔 Chances are that I misunderstand something, but I have not found the reason spelled out so far.

I did not explain myself very well, I meant that Reader relies on desc_chain.readable() whereas Writer relies on desc_chain.writable().

[Ablu]

I think it is still required to separate the desc_chain for reading and for writing.

Why is that? 🤔 Chances are that I misunderstand something, but I have not found the reason spelled out so far.

I did not explain myself very well, I meant that Reader relies on desc_chain.readable() whereas Writer relies on desc_chain.writable().

Ah! Got it now! Thanks for the pointer :). I guess it makes sense with the read-first, write-then specification. In that case, would it maybe make sense to make those readable/writable calls return these reader/write objects? It would require to also do an iterator implementation on them, but it feels like a simpler API to me in the end?

I am slightly worried by forcing the user to manually create the reader / writers, they may not realize that this is possible and instead keep manually iterating things... Or am I too pessimistic here?

[MatiasVara]

I think unless we have some upstream solution, people probably won't put the brain cycles into creating a one-fits-all solutions that suits everyone equally well. I was hoping that one could turn it into a simpler to use API with just a few tweaks. I will try to play with this somewhen during this week and see how it feels.

I agree. The following code sketches what we did for virtio-sound although it was not merged. I am probably missing something here and also this could be refactored. In overall, the read() function is used to read in an continuous way:

pub struct Request {
    pub desc_chain: DescriptorChain,
    pos_curr_desc: usize,
    idx_curr_desc: usize,
}

impl Request {
    pub fn new(len: usize, desc_chain: DescriptorChain) -> Self {
        Self {
            pos_curr_desc: 0,
            idx_curr_desc: 0,
            desc_chain: desc_chain,
        }
    }

    pub fn read(&mut self, buf: &mut [u8]) -> Result<u32> {
        let mut count = buf.len();
        let descriptors: Vec<_> = self.desc_chain.clone().collect();
        let mut buf_pos = 0;

        while count > 0 {
            if self.idx_curr_desc > descriptors.len()  {
                break;
            }
            let avail = descriptors[self.idx_curr_desc].len() - self.pos_curr_desc as u32;
            let mut end = buf.len() as u32;

            if avail < (buf.len() - buf_pos) as u32 {
                end = buf_pos as u32 + avail;
            }

            let len = self
                .desc_chain
                .memory()
                .read(
                    &mut buf[buf_pos..end as usize],
                    descriptors[self.idx_curr_desc]
                        .addr()
                        .checked_add(self.pos_curr_desc as u64)
                        .ok_or(Error::DescriptorReadFailed)?,
                )
                .map_err(|_| Error::DescriptorReadFailed)?;

            buf_pos += len;
            self.pos_curr_desc += len;
            count -= len;

            if self.pos_curr_desc >= descriptors[self.idx_curr_desc].len() as usize {
                self.idx_curr_desc += 1;
                self.pos_curr_desc = 0;
            }
        }
        Ok((buf.len() - count) as u32)
    }
}

Do you think this could be simpler than using Reader/Writer?

[MatiasVara]

I am slightly worried by forcing the user to manually create the reader / writers, they may not realize that this is possible and instead keep manually iterating things... Or am I too pessimistic here?

I think the idea is that user does not use Reader/Writer as iterators. The user will simple issue .read() and .write() from/to other objects like a header.

[Ablu]

I will play with the API a bit later this week. Will respond then :)

[Ablu]

I will play with the API a bit later this week. Will respond then :)

I tried to iterate on it, but did not make as much progress as I hoped while fighting the GuestMemory abstraction layers... Will try to continue tomorrow or Monday...

[Ablu]

Still looking at this... I hit some issues with the generics around the memory abstractions and went into a way to deep rabbit hole to figure out the details. Sorted that out (for now). So back at looking at the actual API.

[Ablu]

Ok. I gave up on the generics for now... It needs some larger refactoring in vm-memory.

So back to the actual topic:

I played with the API a bit. The two things that annoyed me with the reader/writer separation is that code has to manually create the reader/writer and has to forward to the "cursor" to the writable section. Though, I wonder whether we can fix that with two fairly simple tweaks:

1. adding a .reader() and .writer() function on DescriptorChain that returns instances of these helpers
2. have the .writer() automatically advance the chain to the writable descriptors (some daemons currently do this by hardcoding the descriptor layout, some just seek forward X bytes... Just seeking to the first writable descriptor seems easier to me?)

That seems to allow for cutting the remaining boilerplate code while handling descriptors? We probably still need to see how it works in in reality. But I just hacked this for SCSI (which had it's own descriptor chain walking mechanism) and it seems to look reasonable there...

@MatiasVara what do you think?

[MatiasVara]

Ok. I gave up on the generics for now... It needs some larger refactoring in vm-memory.

So back to the actual topic:

I played with the API a bit. The two things that annoyed me with the reader/writer separation is that code has to manually create the reader/writer and has to forward to the "cursor" to the writable section. Though, I wonder whether we can fix that with two fairly simple tweaks:

1. adding a .reader() and .writer() function on DescriptorChain that returns instances of these helpers

Do you mean the Reader and Writer helpers from descriptor_utils.rs? Or the user would be able to use these functions over a DescriptorChain to just read and write over the chain? write() could already take into account the offset due to the read-only descriptors at the begin of the chain.

[Ablu]

Do you mean the Reader and Writer helpers from descriptor_utils.rs?

Yes, I was suggesting new .reader() and .writer() functions on the DescriptorChain that return the helpers added in this PR.

write() could already take into account the offset due to the read-only descriptors at the begin of the chain.

My idea would be the just seek the descriptor chain forward (by simpler calling .next() until we have a writable descriptor) before calling the constructors of the Writer.

Does that make more sense?

[MatiasVara]

Do you mean the Reader and Writer helpers from descriptor_utils.rs?

Yes, I was suggesting new .reader() and .writer() functions on the DescriptorChain that return the helpers added in this PR.

write() could already take into account the offset due to the read-only descriptors at the begin of the chain.

My idea would be the just seek the descriptor chain forward (by simpler calling .next() until we have a writable descriptor) before calling the constructors of the Writer.

Does that make more sense?

Yes, it does, I think you do not need to call .next(), the constructor of Writer already does it.

[Ablu]

Yes, it does, I think you do not need to call .next(), the constructor of Writer already does it.

ah. You are right. I misread the code and thought it yields None if there is a mismatch, but it will just keep iterating and only yields for matching write/read permissions. That looks good then. Not sure if the helper functions are required then...

Shall we try to port a couple more devices to see how it feels?

[MatiasVara]

Yes, it does, I think you do not need to call .next(), the constructor of Writer already does it.

ah. You are right. I misread the code and thought it yields None if there is a mismatch, but it will just keep iterating and only yields for matching write/read permissions. That looks good then. Not sure if the helper functions are required then...

Shall we try to port a couple more devices to see how it feels?

yes, I think so. I would like to try to add the instantiation of Reader/Writer when a DescriptorChain is instantiated (I do not know if you already has this in some repo) and then play with write() and read() in virtio-sound.

[Ablu]

yes, I think so. I would like to try to add the instantiation of Reader/Writer when a DescriptorChain is instantiated (I do not know if you already has this in some repo) and then play with write() and read() in virtio-sound.

I fear tying the reader/writer creation to the constructor of a DescriptorChain may be quite costly. It involves mapping all the buffers even if one then does not need the reader/writer. Also DescriptorChain iterates by yielding instances of itself. We probably do not want to create reader/writers during each iteration step. So I assume we want to keep it "on demand". But we should certainly make it as easy to use as possible.

[MatiasVara]

yes, I think so. I would like to try to add the instantiation of Reader/Writer when a DescriptorChain is instantiated (I do not know if you already has this in some repo) and then play with write() and read() in virtio-sound.

I fear tying the reader/writer creation to the constructor of a DescriptorChain may be quite costly. It involves mapping all the buffers even if one then does not need the reader/writer. Also DescriptorChain iterates by yielding instances of itself. We probably do not want to create reader/writers during each iteration step. So I assume we want to keep it "on demand". But we should certainly make it as easy to use as possible.

Thanks, I think I misunderstood this sentence:

adding a .reader() and .writer() function on DescriptorChain that returns instances of these helpers

I thought that DescriptorChain would instantiate the helpers and .read() and .write() would hide the details about the helpers. But you propose to return something like this in .reader():

Reader::new(&mem, desc_chain.clone())

[Ablu]

I thought that DescriptorChain would instantiate the helpers and .read() and .write() would hide the details about the helpers. But you propose to return something like this in .reader():

Reader::new(&mem, desc_chain.clone())

yep :)

[MatiasVara]

Hello, what would be best approach to fix the CI regarding the coverage? The report states:

ValueError: Current code coverage (86.03%) deviates by -0.95% from the previous code coverage 86.98%.

Thanks.

[Ablu]

Hello, what would be best approach to fix the CI regarding the coverage? The report states:

We should attempt to figure out where it is coming from.

cargo llvm-cov --html

should be able to generate a html report where coverage is annotated line by line. Does that reveal any corner cases that would be useful to cover? Since this is a pure code addition, I assume some lines are simply not covered. If you think there is no reasonable test to fix the coverage then the percentage in the configuration file may be lowered (while making clear in the commit message why there is no point in fixing the coverage).

[MatiasVara]

cargo llvm-cov --html

Thanks, I spotted some functions and corner cases that were not covered. Some of them are:

• split() for writer
• flush()

I will add tests for them.

[stefano-garzarella]

Except for the license (which as I've already written I think is a pre-existing problem to be discussed separately), the rest seems to me to be in very good shape and I will try to use it in vhost-device-vsock soon.

@MatiasVara thanks for this effort!

[Ablu]

Thanks again for working on this! This will greatly simplify descriptor walking and fix a lot of bugs in current code :)

[stefano-garzarella]

I was trying to use Reader and Writer in https://github.com/rust-vmm/vm-virtio/blob/main/virtio-vsock/src/packet.rs, but I needed it to accept a generic GuestMemory, so I gave it a try and after struggling with it, I found a few things that seem to work:

https://github.com/stefano-garzarella/vm-virtio/commit/98497a00fb3b247bd9df70d76576a100cd3dec96

@Ablu @MatiasVara What do you think?

[Ablu]

@Ablu @MatiasVara What do you think?

Ah, I tried the same before but missed being able to set the lifetime via WithBitmapSlice. That certainly fixes the problems that I had where region.find_region() would result in something tied to a lifetime to the region (and thus not being able to add it to the Writer instance).

So it looks good to me. Yet, this interface in vm-memory is just way too complex to use without loosing sanity. I also dislike the Deref use in DescriptorChain. It disallows properly using lifetimes through it. IMHO it should just be a &GuestMemory there. But I failed refactoring this in sensible steps as everything is so intertwined.

[stefano-garzarella]

@Ablu @MatiasVara What do you think?

Ah, I tried the same before but missed being able to set the lifetime via WithBitmapSlice. That certainly fixes the problems that I had where region.find_region() would result in something tied to a lifetime to the region (and thus not being able to add it to the Writer instance).

Yeah, I was inspired by virtio-vsock API: https://github.com/rust-vmm/vm-virtio/blob/d219302978ddaee67bd341433e8ac219da0302b8/virtio-vsock/src/packet.rs#L418

BTW, in the end I think I'll change the virtio-vsock API to accept Reader and Writer from the caller, so I'm not sure if I really need the changes I proposed. But if you think they can improve a bit this PR, feel free to incorporate.

So it looks good to me. Yet, this interface in vm-memory is just way too complex to use without loosing sanity. I also dislike the Deref use in DescriptorChain. It disallows properly using lifetimes through it. IMHO it should just be a &GuestMemory there. But I failed refactoring this in sensible steps as everything is so intertwined

Okay, so I'm not the only one who almost went crazy with it....

[MatiasVara]

I was trying to use Reader and Writer in https://github.com/rust-vmm/vm-virtio/blob/main/virtio-vsock/src/packet.rs, but I needed it to accept a generic GuestMemory, so I gave it a try and after struggling with it, I found a few things that seem to work:

stefano-garzarella@98497a0

@Ablu @MatiasVara What do you think?

I think is it is a good idea. I just pushed a version with the changes.

[stefano-garzarella]

I'm going to merge this, since we want to use it in vhost-devices. We have 3 acks, so it should be okay.