firecracker icon indicating copy to clipboard operation
firecracker copied to clipboard

Clean up `create_fdt` function in `src/vmm/src/arch/aarch64/fdt.rs`

Open roypat opened this issue 1 year ago • 6 comments

The create_fdt function has two eccentricities that can be cleaned up:

  1. It is unnecessarily parametrized by std::hash::HashBuilder, and
  2. It both writes the flattened device tree to guest memory, as well as returns it (however this return value is ignored the only call site). In the spirit of separating concerns, it should not write the FDT to guest memory - instead the caller should use the return value and write it to memory. This might also simplify our unittesting a little bit, as we no longer need to construct GuestMemoryMmap in the tests.

See also https://github.com/firecracker-microvm/firecracker/pull/4687#discussion_r1686789518

roypat avatar Jul 23 '24 11:07 roypat

Hey, @roypat!

I'd like to work on this issue, can you assign me?

Thanks!

jackabald avatar Jul 23 '24 16:07 jackabald

Hi @jackabald,

I've assigned you, thanks for having a look!

roypat avatar Jul 24 '24 06:07 roypat

@roypat, I have made changes to suit your needs regarding this issue. However, I am having trouble configuring the test cases in fdt.rs. I don't want to open a pull request without fully testing this code, but I am having trouble running these test cases.

Should I just send a pull request for you to take a look at?

jackabald avatar Jul 25 '24 19:07 jackabald

@roypat, I have made changes to suit your needs regarding this issue. However, I am having trouble configuring the test cases in fdt.rs. I don't want to open a pull request without fully testing this code, but I am having trouble running these test cases.

Should I just send a pull request for you to take a look at?

Yes, opening a draft PR for early feedback is always welcome :)

roypat avatar Jul 26 '24 08:07 roypat

@roypat linked the pull request. please let me know if I am going in the right direction. I had created a whole new function for writing to guest memory but it seems it's not needed if we just want callers to assign it to memory based on its return value.

jackabald avatar Jul 26 '24 18:07 jackabald

Please review my new pull request. If you would like me to work on cleaning up some of the unit testing I will happily work on this further :)

Thank you!

jackabald avatar Jul 31 '24 06:07 jackabald

The PR that fixed this was merged last July, closing this issue :)

roypat avatar Feb 19 '25 09:02 roypat