Reorganize code into x86_64/{elf, bzimage} and aarch64 modules #24

alxiord · 2020-03-10T12:21:17Z

The first 2 commits in this PR split the code that currently bloats src/loader/mod.rs into:

dedicated elf and bzimage modules under loader
per-arch modules:
1. x86_64/elf
2. x86_64/bzimage
3. aarch64

Overall, the purpose of this reorganization is to improve readability and maintainability by keeping src/loader/mod.rs simple.

src/loader/x86_64/elf/mod.rs

serban300

Looks good to me. I have only one question. Is start_info.rs needed ? It doesn't seem to be used inside the crate.

alxiord · 2020-03-16T13:56:52Z

Looks good to me. I have only one question. Is start_info.rs needed ? It doesn't seem to be used inside the crate.

#11 (comment) explains why this crate should export the structs in start_info and how the VMM that imports linux-loader should use them.

andreeaflorescu · 2020-03-18T11:49:30Z

nit: Can we organize the exports as they're organized in vmm-sys-util? You can check src/lib.rs and the mod.rs file from unix. The advantage with that approach is that when you add a new feature you don't have to grow the lib.rs file and have both the attributes for the features & for the platform at the same level.

alxiord · 2020-03-18T15:19:07Z

nit: Can we organize the exports as they're organized in vmm-sys-util? You can check src/lib.rs and the mod.rs file from unix. The advantage with that approach is that when you add a new feature you don't have to grow the lib.rs file and have both the attributes for the features & for the platform at the same level.

I don't understand which structure you have in mind for lib.rs. It only exports pub mod loader, so it's pretty small. loader/mod.rs is more complicated because the error and KernelLoaderResult types there aggregates errors from all the other mods... I can't figure out a way to simplify the imports.

alxiord · 2020-03-18T16:05:57Z

@rbradford / @sameo / @sboeuf Could either of you PTAL too?

sameo · 2020-03-18T18:24:04Z

src/loader/mod.rs

+#[cfg(all(feature = "elf", any(target_arch = "x86", target_arch = "x86_64")))]
+pub use x86_64::elf::Elf;
+#[cfg(all(feature = "elf", any(target_arch = "x86", target_arch = "x86_64")))]
+pub use x86_64::elf::Error as ElfError;


I think you also need a:

#[cfg(all(feature = "elf", any(target_arch = "x86", target_arch = "x86_64")))] pub use x86_64::elf::start_info;

I think (hope) that both this issue and @andreeaflorescu's nit #24 (comment) are solved in the latest commit. The elf and bzimage modules are used as-is, simplifying the imports in loader/mod.rs, at the expense of the user prepending elf:: and bzimage:: to structs and calls.

In preparation for more incoming code that's either elf or bzimage specific, the respective blocks of functionality now sit in separate modules. This makes the code easier to navigate and improves readability. Fixes rust-vmm#22 as well. Signed-off-by: Alexandra Iordache <[email protected]>

alxiord · 2020-03-23T17:24:26Z

@iulianbarbu @serban300 @sameo @andreeaflorescu Lost all approvals, rebased & updated, PTAL 😃

sameo · 2020-03-24T07:06:46Z

Works with Cloud Hypervisor.

src/loader/bzimage/mod.rs

src/loader/elf/mod.rs

src/loader/mod.rs

Phase 2 of code reorganization: separate x86_64 from aarch64 functionality in modules. Signed-off-by: Alexandra Iordache <[email protected]>

Signed-off-by: Alexandra Iordache <[email protected]>

alxiord · 2020-03-24T18:29:20Z

@iulianbarbu @serban300 @sameo @andreeaflorescu Lost all approvals, rebased & updated, PTAL

Lost again but I hope this is the last time I ask for re-approvals

sboeuf · 2020-03-25T08:30:21Z

Lost again but I hope this is the last time I ask for re-approvals

Do you think we could disable this feature? I mean unless we really don't trust the people submitting code, I don't expect someone to shove some bad code on purpose between two iterations of a PR.
Moreover, if someone feel like the PR is not ready while someone else already approved it, it's only a matter of clicking the Request Changes button when doing the review. This should prevent the PR from being merged.
What do y'all think?

andreeaflorescu · 2020-03-25T08:50:06Z

@sboeuf I am a bit on the fence about this one. The thing is that you can make mistakes on rebases so someone should still look at the code to make sure you didn't introduce any bugs.

Ideally, you would need to re-approve it only when the PR has changes that were not rebase specific. GitHub helps with that by allowing you to click the update branch button that does rebases for you while keeping the approves. The problem is that with PRs that sit a lot in the queue before getting merged, you will have conflicts that GitHub doesn't know how to fix. The same happens for PRs that change lots of lines of code (like this one).

How about before taking a decision, we put some metrics in place to see how often it happens that we need re-approvals where they would not be necessary and take a data-driven decision? I personally don't feel comfortable with taking that decision now.

I would also move this conversation either in a community issue or on the email so that it doesn't get lost when this PR is merged.

sboeuf · 2020-03-25T09:19:24Z

@andreeaflorescu

The thing is that you can make mistakes on rebases so someone should still look at the code to make sure you didn't introduce any bugs.

That's why we put our trust in the CI :)

How about before taking a decision, we put some metrics in place to see how often it happens that we need re-approvals where they would not be necessary and take a data-driven decision? I personally don't feel comfortable with taking that decision now.

Of course, there's no rush, but that's something I find a bit heavy in terms of process, and this PR is the perfect example, that's why I wanted to raise the question here.

I would also move this conversation either in a community issue or on the email so that it doesn't get lost when this PR is merged.

Sounds good!

alxiord force-pushed the cleanup branch from 5925637 to fcafefa Compare March 10, 2020 13:18

alxiord self-assigned this Mar 10, 2020

alxiord requested a review from andreeaflorescu March 10, 2020 13:18

alxiord mentioned this pull request Mar 10, 2020

Add support for PVH boot protocol #11

Merged

alxiord force-pushed the cleanup branch 3 times, most recently from bd6b09e to d6f52c7 Compare March 14, 2020 12:27

iulianbarbu reviewed Mar 14, 2020

View reviewed changes

src/loader/x86_64/elf/mod.rs Show resolved Hide resolved

src/loader/x86_64/elf/mod.rs Show resolved Hide resolved

serban300 previously approved these changes Mar 16, 2020

View reviewed changes

alxiord dismissed serban300’s stale review via 272f3b9 March 18, 2020 08:06

iulianbarbu previously approved these changes Mar 18, 2020

View reviewed changes

andreeaflorescu previously approved these changes Mar 18, 2020

View reviewed changes

serban300 previously approved these changes Mar 18, 2020

View reviewed changes

sameo reviewed Mar 18, 2020

View reviewed changes

alxiord dismissed stale reviews from serban300, andreeaflorescu, and iulianbarbu via b597cf9 March 18, 2020 18:53

alxiord mentioned this pull request Mar 23, 2020

replace struct_util with ByteValued from vm-memory #26

Merged

alxiord force-pushed the cleanup branch from b597cf9 to 6a68bff Compare March 23, 2020 16:57

serban300 previously approved these changes Mar 23, 2020

View reviewed changes

sameo previously approved these changes Mar 24, 2020

View reviewed changes

andreeaflorescu reviewed Mar 24, 2020

View reviewed changes

src/loader/bzimage/mod.rs Outdated Show resolved Hide resolved

src/loader/elf/mod.rs Outdated Show resolved Hide resolved

src/loader/mod.rs Outdated Show resolved Hide resolved

add x86_64 and aarch64 modules

63fceb9

Phase 2 of code reorganization: separate x86_64 from aarch64 functionality in modules. Signed-off-by: Alexandra Iordache <[email protected]>

Alexandra Iordache added 2 commits March 24, 2020 16:48

buildkite: update hook for new bzimage location

664b3da

Signed-off-by: Alexandra Iordache <[email protected]>

elf: add missing documentation

388fd99

Signed-off-by: Alexandra Iordache <[email protected]>

alxiord dismissed stale reviews from sameo and serban300 via 388fd99 March 24, 2020 14:48

alxiord force-pushed the cleanup branch from 6a68bff to 388fd99 Compare March 24, 2020 14:48

andreeaflorescu approved these changes Mar 24, 2020

View reviewed changes

serban300 approved these changes Mar 24, 2020

View reviewed changes

sameo approved these changes Mar 26, 2020

View reviewed changes

sameo merged commit 2adddce into rust-vmm:master Mar 26, 2020

alxiord deleted the cleanup branch March 26, 2020 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize code into x86_64/{elf, bzimage} and aarch64 modules #24

Reorganize code into x86_64/{elf, bzimage} and aarch64 modules #24

alxiord commented Mar 10, 2020 •

edited

Loading

serban300 left a comment

alxiord commented Mar 16, 2020

andreeaflorescu commented Mar 18, 2020

alxiord commented Mar 18, 2020

alxiord commented Mar 18, 2020

sameo Mar 18, 2020

alxiord Mar 18, 2020 •

edited

Loading

alxiord commented Mar 23, 2020

sameo commented Mar 24, 2020

alxiord commented Mar 24, 2020

sboeuf commented Mar 25, 2020

andreeaflorescu commented Mar 25, 2020

sboeuf commented Mar 25, 2020

Reorganize code into x86_64/{elf, bzimage} and aarch64 modules #24

Reorganize code into x86_64/{elf, bzimage} and aarch64 modules #24

Conversation

alxiord commented Mar 10, 2020 • edited Loading

serban300 left a comment

Choose a reason for hiding this comment

alxiord commented Mar 16, 2020

andreeaflorescu commented Mar 18, 2020

alxiord commented Mar 18, 2020

alxiord commented Mar 18, 2020

sameo Mar 18, 2020

Choose a reason for hiding this comment

alxiord Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

alxiord commented Mar 23, 2020

sameo commented Mar 24, 2020

alxiord commented Mar 24, 2020

sboeuf commented Mar 25, 2020

andreeaflorescu commented Mar 25, 2020

sboeuf commented Mar 25, 2020

alxiord commented Mar 10, 2020 •

edited

Loading

alxiord Mar 18, 2020 •

edited

Loading