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.

Sign up for GitHub

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

Jump to bottom

Simplified disk builder #320

Merged
merged 33 commits into from
Mar 12, 2023
Merged

Simplified disk builder #320

merged 33 commits into from
Mar 12, 2023

Conversation

jasoncouture
Copy link
Contributor

@jasoncouture jasoncouture commented Jan 9, 2023 •
edited
Loading

Simplify the disk builder:

  1. Move UEFI and BIOS into a common implementation
  2. Allow adding of arbitrary files

Closes #321

Note: No new tests, as existing tests fully cover these changes.

@jasoncouture jasoncouture force-pushed the simplified_disk_builder branch from e28e4ae to 73f599d Compare January 9, 2023 18:08
@jasoncouture jasoncouture mentioned this pull request Jan 9, 2023
Closed
@jasoncouture jasoncouture force-pushed the simplified_disk_builder branch 3 times, most recently from 625f3b6 to 7d6715e Compare January 11, 2023 00:31
@jasoncouture
Copy link
Contributor Author

@phil-opp this should also be ready to go.

@phil-opp
Copy link
Member

I only skimmed this PR, but I like the general approach. Some notes:

  • Removing the BiosBoot and UefiBoot structs is a breaking change, so we cannot release this PR in a v0.11 patch version. So we have two options:
    • Merge this PR once we plan to release v0.12.
    • Keep BiosBoot and UefiBoot and just implement their methods on top of top of the new DiskImageBuilder.
  • We don't expect that many files on the FAT partition, so I think cloning the DiskImageFile fields should be fine. This would allow us to remove the lifetime from the struct and make the whole API simpler.
  • Nit: Instead of &PathBuf, it's recommended to take &Path as argument.
  • The changes in build.rs seem unrelated to this PR. Removing these changes should also fix the conflict.

@jasoncouture
Copy link
Contributor Author

I only skimmed this PR, but I like the general approach. Some notes:

  • Removing the BiosBoot and UefiBoot structs is a breaking change, so we cannot release this PR in a v0.11 patch version. So we have two options:

    • Merge this PR once we plan to release v0.12.
    • Keep BiosBoot and UefiBoot and just implement their methods on top of top of the new DiskImageBuilder.

Why not both? Let's keep *Boot, as wrappers on top of DiskImageBuilder, and in 0.12 nuke them?

  • We don't expect that many files on the FAT partition, so I think cloning the DiskImageFile fields should be fine. This would allow us to remove the lifetime from the struct and make the whole API simpler.

I'll look into it :)
Even if there was a ton of files, do we really need the builder to be high performance?

  • Nit: Instead of &PathBuf, it's recommended to take &Path as argument.

Will do.

  • The changes in build.rs seem unrelated to this PR. Removing these changes should also fix the conflict.

They are a remnant of previous rebases, I'll ditch the changes.

@phil-opp
Copy link
Member

Why not both? Let's keep *Boot, as wrappers on top of DiskImageBuilder, and in 0.12 nuke them?

Sure, that would be fine.

Even if there was a ton of files, do we really need the builder to be high performance?

Faster builds are always nice, but it's not the first priority. What I meant is that it should be fine to call clone() in this implementation in order to simplify the API.

@phil-opp
Copy link
Member

Another thought: Maybe we should add an additional builder method to add files in form of a Vec<u8> (instead of &Path). This would be useful for writing files that are generated on-the-fly without needing to write them to some temporary file first. One example could be some kernel configuration that is serialized using serde.

@jasoncouture
Copy link
Contributor Author

Another thought: Maybe we should add an additional builder method to add files in form of a Vec<u8> (instead of &Path). This would be useful for writing files that are generated on-the-fly without needing to write them to some temporary file first. One example could be some kernel configuration that is serialized using serde.

Agreed. I haven't had a ton of spare time lately, I should be able to get back on this this weekend.

@jasoncouture jasoncouture force-pushed the simplified_disk_builder branch 2 times, most recently from d48d8f4 to d19228d Compare January 29, 2023 01:36
@jasoncouture jasoncouture reopened this Jan 29, 2023
phil-opp
phil-opp reviewed Jan 29, 2023
View reviewed changes
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@jasoncouture jasoncouture force-pushed the simplified_disk_builder branch from 76ca9e9 to a48f2ef Compare March 4, 2023 21:28
@jasoncouture jasoncouture force-pushed the simplified_disk_builder branch from a48f2ef to d4e9a5f Compare March 4, 2023 23:24
jasoncouture and others added 18 commits March 4, 2023 18:26
@jasoncouture
Refactor set methods to take destination first
42b5895
@jasoncouture
Fix warnings with
dd87c00
@jasoncouture
Add documentation comments to set_file_* methods
3a7fcf7
@jasoncouture
Add documentation comments to FileDataSource and it's public methods.
502967b
@jasoncouture
Remove code comments to pass doc test.
ad34510
@jasoncouture
Final formatting pass
a93ce32
@jasoncouture
Remove DiskImageFile in favor of a BTreeMap
3ce9598
@phil-opp
Improve docs
6b8d970
@phil-opp
Make set_file_source private
5cabc0c 
We don't want to expose the `FileDataSource` enum for now.
@phil-opp
Take arguments as owned values in set_file and set_file_contents
2923ad8
@phil-opp
Reorder methods (public first)
1a5cdf6
@phil-opp
Document that only the kernel and ramdisk are loaded into memory
6e9b6a3
@phil-opp
Apply clippy suggestion
c4714ce
@phil-opp
Take FileDataSource by reference in create_fat_filesystem
075f22d
@phil-opp
Serialize boot config to Vec directly
692d39f
@phil-opp
Take file paths by value to avoid internal cloning
bd5047c 
Also: Store file paths as `Cow<'static, str>` internally to avoid cloning for const paths.
@phil-opp
Follow-up fixes for by-value arguments
f0328e6
@phil-opp
Adjust test runner for new owned arguments
ac934c4
phil-opp
phil-opp approved these changes Mar 12, 2023
View reviewed changes
Copy link
Member

@phil-opp phil-opp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the updates! Looks good now!

I pushed some more commits with a few minor tweaks.

@phil-opp phil-opp enabled auto-merge March 12, 2023 14:06
@phil-opp phil-opp merged commit 8d2d53c into rust-osdev:main Mar 12, 2023
@phil-opp phil-opp mentioned this pull request Mar 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-opp phil-opp phil-opp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disk builder support for arbitrary files
2 participants
@jasoncouture @phil-opp