Add GuestMemory method to return an Iterator

[Daniel-Aaron-Bloom]

Fixes #8.

This changes is similar to #9, which was rejected as it introduced a lifetime parameter to the trait, in favor of waiting for GAT/GAL. Since GAT/GAL still looks a ways off, this change does the same thing but using HRTBs.

[Daniel-Aaron-Bloom]

Should I make a separate PR for all the clippy fixes and miscellanea?

[bonzini]

It's okay to keep them in this pull request, but please move those that apply to the master branch in a separate commit; the others should be squashed in the commit that introduces the problem in the first place.

Also, to complete my review:

• I would leave the map_and_fold and with_regions* methods in order to avoid breaking API compatibility

• Does the pub struct Iter<'a> for Guest memory map need to be public at all? If not, I would rather hide it.

• the GuestMemoryIter trait needs a more complete example in the documentation.

[Daniel-Aaron-Bloom]

I would leave the map_and_fold and with_regions* methods in order to avoid breaking API compatibility

Sounds good to me. I'll make this change.

the GuestMemoryIter trait needs a more complete example in the documentation.

There is no runtime usage for GuestMemoryIterator. It is only a dummy trait which enables lifetime HKTs in order to setup the returned iterator type with the proper lifetime. I'll try to improve the documentation, but more guidance on this point would be appreciated.

Does the pub struct Iter<'a> for Guest memory map need to be public at all? If not, I would rather hide it.

It does need to be public as users may need to store and wrap iterators.

It's okay to keep them in this pull request, but please move those that apply to the master branch in a separate commit; the others should be squashed in the commit that introduces the problem in the first place.

Since most (almost all) of these issues existed before, I've separated out the fixes for pre-existing clippy issues into #131. That should probably go through first and clear up the CI, then I'll rebase this and cleanup the commit history (along with the other requested changes)

[jiangliu]

LGTM:)
Could you please also help to handle GuestMemoryAtomic in atomic.rs? Otherwise it breaks.

[Daniel-Aaron-Bloom]

Could you please also help to handle GuestMemoryAtomic in atomic.rs? Otherwise it breaks.

I'm confused. GuestMemoryAtomic doesn't implement the GuestMemory interface. The associated guards implement Deref, but that should work cleanly to access the new methods.

[jiangliu]

Could you please also help to handle GuestMemoryAtomic in atomic.rs? Otherwise it breaks.

I'm confused. GuestMemoryAtomic doesn't implement the GuestMemory interface. The associated guards implement Deref, but that should work cleanly to access the new methods.

You are right:)

[bonzini]

Regarding documentation, I would like something like

    /// Lifetime generic associated iterator. Usually this is just `Self`.  The actual
    /// iterator type is specified through the `GuestMemoryIterator` trait, for example:
    ///
    /// ```ignore
    /// impl<'a> GuestMemoryIterator<'a, MyGuestRegion> for MyGuestMemory {
    ///     type Iter = MyGuestMemoryIter<'a>;
    /// }
    ///
    /// pub struct MyGuestMemoryIter<`a>( ... );
    /// impl<'a> Iterator for MyGuestMemoryIter<'a> { ... }
    /// ```
    type Iterator: for<'a> GuestMemoryIterator<'a, Self::R>;

that explains how to use the idiom.

Finally, since it is public please rename the Iter type to GuestMemoryMmapIter. Thanks!

[alexandruag]

This is neat! I think we can avoid explicitly renaming the mmap::Iter type to GuestMemoryMmapIter, since the module path already provides sufficient context (this also happens for some of the iterators in the standard library, i.e. slice::Iter).

It's definitely a good idea to keep the old methods around some more to avoid immediately breaking compatibility, but I think we should remove them at some point in the future. What do you folks think about marking them as deprecated for now, and removing them in a later release?

Finally, we should also add some changelog entries about the publicly visible changes (in an [Unreleased] section right before [0.4.0], that's going to be renamed using the proper version number when we prepare the next release).

[Daniel-Aaron-Bloom]

I'm also considering adding a fn find_regions(&self, start: GuestAddress, end: GuestAddress) -> <Self::I as GuestMemoryIterator<Self::R>>::Regions to support rust-vmm/vm-virtio#33. Specifically in regards to @alexandruag comment on

getting the minimal sequence of VolatileSlices that makes up an otherwise contiguous (and valid) GPA range

Am I correct that a find_regions would help resolve that issue?

[bonzini]

Just one remaining comment for the documentation.

[alexandruag]

I'm also considering adding a fn find_regions(&self, start: GuestAddress, end: GuestAddress) -> <Self::I as GuestMemoryIterator<Self::R>>::Regions to support rust-vmm/vm-virtio#33. Specifically in regards to @alexandruag comment on

getting the minimal sequence of VolatileSlices that makes up an otherwise contiguous (and valid) GPA range

Am I correct that a find_regions would help resolve that issue?

Apologies for missing this comment earlier. In my mind, the question has increased in scope recently, for example also touching on the subject of how we can leverage something along these lines to simplify how cross-region accesses are performed. I'll open an issue as soon as some sort of idea does eventually crystallize, and hopefully we can reduce overall complexity some more. I'm also more than happy to discuss other ideas as well.