replace struct_util with ByteValued from vm-memory

[alxiord]

#20, rust-vmm/vmm-sys-util#77

Even though there's more code at the moment, it will be simplified once rust-vmm/vm-memory#83 is merged and released. See https://github.com/rust-vmm/vmm-sys-util/issues/77#issuecomment-597576916 for a preview.

[serban300]

Now there seems to be some duplicated logic, because volatile_slice::read_from() doesn't completely replace the logic in struct_util. It might make it safer, but it's just a building block for those helper methods.

So I wouldn't remove struct_util. We can refactor it, but I would keep those helper functions somehow. Maybe we can use only 1 helper function instead of 2.

[alxiord]

Now there seems to be some duplicated logic, because volatile_slice::read_from() doesn't completely replace the logic in struct_util. It might make it safer, but it's just a building block for those helper methods.

So I wouldn't remove struct_util. We can refactor it, but I would keep those helper functions somehow. Maybe we can use only 1 helper function instead of 2.

I'd like to work incrementally towards this: https://github.com/rust-vmm/vmm-sys-util/issues/77#issuecomment-597576916
This will effectively obsolete struct_util::read_struct and read_struct_slice can be replaced with the same thing in a loop. This PR would be an intermediate point between nice code with struct_util and nice code with vm-memory::Bytes. The code is ugly in the intermediate point, yes, but it's a wip until a new vm-memory release is available with the added knack that makes the elegant solution possible: rust-vmm/vm-memory#83
Would you agree to the temporary ugly code? If not, I'd replace the implementation in struct_util to leverage vm-memory, then when vm-memory is released, get rid of struct_util.

[serban300]

Now there seems to be some duplicated logic, because volatile_slice::read_from() doesn't completely replace the logic in struct_util. It might make it safer, but it's just a building block for those helper methods.
So I wouldn't remove struct_util. We can refactor it, but I would keep those helper functions somehow. Maybe we can use only 1 helper function instead of 2.

I'd like to work incrementally towards this: rust-vmm/vmm-sys-util#77 (comment)
This will effectively obsolete struct_util::read_struct and read_struct_slice can be replaced with the same thing in a loop. This PR would be an intermediate point between nice code with struct_util and nice code with vm-memory::Bytes. The code is ugly in the intermediate point, yes, but it's a wip until a new vm-memory release is available with the added knack that makes the elegant solution possible: rust-vmm/vm-memory#83
Would you agree to the temporary ugly code? If not, I'd replace the implementation in struct_util to leverage vm-memory, then when vm-memory is released, get rid of struct_util.

Looks good. I didn't know about this. Anyway, I think there is a problem. In order to get there we need to implement VolatileMemory for ByteValued:

impl<T:ByteValued> VolatileMemory for T {
    fn len(&self) -> usize {
        self.as_slice().len()
    }

    fn get_slice(&self, offset: usize, count: usize) -> Result<VolatileSlice<'_>> {
        Ok(self.as_bytes())
    }
}

But get_slice takes an immutable reference to self while as_bytes needs a mutable one. We can modify get_slice to take a mut ref to self, but this generates a lot of related changes. We can also do: self as *const Self as *mut Self inside the as_bytes() impementation. I don't know if any one of this solutions is acceptable.

Personally I would keep struct_util for the moment and rewrite those methods using as_volatile_memory. After this problem is solved in vm-memory and a new version of the crate is published we can come back to this and remove struct_util.

[bonzini]

In order to get there we need to implement VolatileMemory for ByteValued:

No, as_bytes() returns a VolatileSlice, which implements Bytes<usize>. You don't need to implement VolatileMemory anywhere. You should be able to do

    let mut ehdr: elf::Elf64_Ehdr = Default::default();
    ehdr.as_bytes().read_from(0, kernel_image, ehdr.len()).map_err(|_| Error::ReadElfHeader)?;

[serban300]

let mut ehdr: elf::Elf64_Ehdr = Default::default();
    ehdr.as_bytes().read_from(0, kernel_image, ehdr.len()).map_err(|_| Error::ReadElfHeader)?;

You're right. Sorry, I misunderstood something.

[andreeaflorescu]

vm-memory v0.2.0 is now published on crates.io. We should be able to replace struct_util now.

[rbradford]

Version bump needed should be ^0, >=0.2.0 as @bonzini has suggested.

[alxiord]

@rbradford @andreeaflorescu the vm-memory dependency is updated to >=0.2.0 in the latest version, which also makes use of the newly released functionality in vm-memory.

[rbradford]

@aghecenco you just need to bump coverage

[andreeaflorescu]

nit: we can have these cfgs as #[cfg(all(feature = "elf", any(target_arch = "x86", target_arch = "x86_64"))].

[alxiord]

Most of these are fixed in #24

[alxiord]

@rbradford coverage updated, PTAL