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

Add struct_util module from linux-loader #88

Closed
aghecenco opened this issue Mar 10, 2020 · 6 comments
Closed

Add struct_util module from linux-loader #88

aghecenco opened this issue Mar 10, 2020 · 6 comments
Assignees

Comments

@aghecenco
Copy link
Member

See also rust-vmm/linux-loader#20, rust-vmm/linux-loader#23 (comment)

struct_util is a generic utility module that reads structs from bytes. It should be included in this utility crate and consumed by linux-loader from here.

@aghecenco aghecenco self-assigned this Mar 10, 2020
aghecenco referenced this issue in aghecenco/vmm-sys-util Mar 10, 2020
Fixes #77

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
@bonzini
Copy link
Member

bonzini commented Mar 10, 2020

There is some overlap between this and vm-memory's Bytes and ByteValued traits. Can you try instead implementing ByteValued on the structs and using the read_obj/write_obj methods from the Bytes trait? The main limitation is that you need a mutable reference to slice but it shouldn't be a problem for linux-loader.

@aghecenco
Copy link
Member Author

aghecenco commented Mar 11, 2020

Indeed we can impl ByteValued on the headers currently read with struct_util, and that gets us elegantly rid of struct_util; later on, the GuestMemory reads the whole kernel image, not just the header, as a generic F: Read (for elf format: link).

We'd be replacing

let mut ehdr: elf::Elf64_Ehdr = Default::default();
unsafe { struct_util::read_struct(kernel_image, &mut ehdr).map_err(|_| Error::ReadElfHeader)?; }

with

let mut ehdr_bytes = vec![0u8; mem::size_of::<elf::Elf64_Ehdr>()];
kernel_image.read_exact(ehdr_bytes.as_mut_slice()).map_err(|_| Error::ReadElfHeader)?;
let ehdr = elf::Elf64_Ehdr::from_mut_slice(ehdr_bytes.as_mut_slice()).ok_or(Error::ReadElfHeader)?;

which I don't necessarily find more elegant, but personally prefer rather than move struct_util around and make it pub.
It might be more elegant if kernel_image were a Bytes and not just a Read + Seek, so we could read_obj from it directly, but I think that would be too restrictive on linux-loader.

@bonzini
Copy link
Member

bonzini commented Mar 11, 2020

Yes, it's not super nice but at least it doesn't have unsafe. You can also put ehdr_bytes.as_mut_slice() into a local variable which is a bit nicer. Or perhaps use an array like

let mut ehdr_bytes = [0u8; mem::size_of::<elf::Elf64_Ehdr>()];
kernel_image.read_exact(&mut ehdr_bytes[..]).map_err(|_| Error::ReadElfHeader)?;
let ehdr = elf::Elf64_Ehdr::from_mut_slice(&mut ehdr_bytes[..]).ok_or(Error::ReadElfHeader)?;

?

@bonzini
Copy link
Member

bonzini commented Mar 11, 2020

Also:

let mut ehdr_bytes = [0u8; mem::size_of::<elf::Elf64_Ehdr>()];
ehdr_bytes.as_volatile_slice().read_from(0, kernel_image, ehdr_bytes.len()).map_err(|_| Error::ReadElfHeader)?;
let ehdr = elf::Elf64_Ehdr::from_mut_slice(&mut ehdr_bytes[..]).ok_or(Error::ReadElfHeader)?;

... and finally, we could implement VolatileMemory on ByteValued (#83) and do

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

@aghecenco
Copy link
Member Author

The last solution looks great and I find it to be the most elegant alternative to struct_util. Until #83 is merged & released, I'll go ahead with the other solution.

aghecenco referenced this issue in aghecenco/linux-loader Mar 11, 2020
Fixes rust-vmm#20
Closes rust-vmm/vmm-sys-util#77

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
aghecenco referenced this issue in aghecenco/linux-loader Mar 11, 2020
Fixes rust-vmm#20
Closes rust-vmm/vmm-sys-util#77

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
@bonzini bonzini closed this as completed Mar 12, 2020
@andreeaflorescu andreeaflorescu transferred this issue from rust-vmm/vmm-sys-util Mar 18, 2020
@andreeaflorescu
Copy link
Member

Transferred this issue from vmm-sys-util to vm-memory as the implementation is in vm-memory now.

Fix introduced in: #83

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

No branches or pull requests

3 participants