Simplified disk builder #320

jasoncouture · 2023-01-09T18:06:27Z

Simplify the disk builder:

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

Closes #321

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

jasoncouture · 2023-01-11T01:28:46Z

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

phil-opp · 2023-01-11T11:59:43Z

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 · 2023-01-14T01:40:33Z

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 · 2023-01-16T16:58:24Z

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 · 2023-01-16T17:01:07Z

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 · 2023-01-17T20:02:16Z

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.

phil-opp

Thanks for the updates!

uefi/src/main.rs

src/lib.rs

…d convert PathBuf parameters to Path

This also renames the methods to add files to set_.

We don't want to expose the `FileDataSource` enum for now.

Also: Store file paths as `Cow<'static, str>` internally to avoid cloning for const paths.

phil-opp

Thanks a lot for the updates! Looks good now!

I pushed some more commits with a few minor tweaks.

jasoncouture force-pushed the simplified_disk_builder branch from e28e4ae to 73f599d Compare January 9, 2023 18:08

jasoncouture mentioned this pull request Jan 9, 2023

Disk builder support for arbitrary files #321

Closed

jasoncouture force-pushed the simplified_disk_builder branch 3 times, most recently from 625f3b6 to 7d6715e Compare January 11, 2023 00:31

jasoncouture force-pushed the simplified_disk_builder branch 2 times, most recently from d48d8f4 to d19228d Compare January 29, 2023 01:36

jasoncouture closed this Jan 29, 2023

jasoncouture reopened this Jan 29, 2023

phil-opp reviewed Jan 29, 2023

View reviewed changes

jasoncouture force-pushed the simplified_disk_builder branch from 76ca9e9 to a48f2ef Compare March 4, 2023 21:28

jasoncouture added 13 commits March 4, 2023 16:29

Replace UefiBoot and BiosBoot with DiskImageBuilder

164d015

Update DiskImageBuilder to remove lifetime requirement by cloning, an…

85779d3

…d convert PathBuf parameters to Path

Reimplement BiosBoot and UefiBoot as wrappers around DiskImageBuilder

5786d9b

cargo fmt

131130b

Add abi_efiapi feature

2a7a25c

Fix build errors due to rebase

25536d2

Fix clippy failures

b472297

Remove unnecessary feature abi_efiapi

d1a2391

Rename image_filename to image_path

37c5539

Create enum to hold different sources of file data

2af185f

Update code to use FileDataSource

81e9d80

Format code

c0fab37

Add public method to set source directly

c193f9f

This also renames the methods to add files to set_.

Format files after changes

d4e9a5f

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

Refactor set methods to take destination first

42b5895

Fix warnings with

dd87c00

Add documentation comments to set_file_* methods

3a7fcf7

Add documentation comments to FileDataSource and it's public methods.

502967b

Remove code comments to pass doc test.

ad34510

Final formatting pass

a93ce32

Remove DiskImageFile in favor of a BTreeMap

3ce9598

Improve docs

6b8d970

Make set_file_source private

5cabc0c

We don't want to expose the `FileDataSource` enum for now.

Take arguments as owned values in set_file and set_file_contents

2923ad8

Reorder methods (public first)

1a5cdf6

Document that only the kernel and ramdisk are loaded into memory

6e9b6a3

Apply clippy suggestion

c4714ce

Take FileDataSource by reference in create_fat_filesystem

075f22d

Serialize boot config to Vec directly

692d39f

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.

Follow-up fixes for by-value arguments

f0328e6

Adjust test runner for new owned arguments

ac934c4

phil-opp approved these changes Mar 12, 2023

View reviewed changes

Fix unused import

e3dd6fc

phil-opp enabled auto-merge March 12, 2023 14:06

phil-opp merged commit 8d2d53c into rust-osdev:main Mar 12, 2023

phil-opp mentioned this pull request Mar 12, 2023

Release version 0.11.1 #350

Merged

Simplified disk builder #320

Simplified disk builder #320

Uh oh!

Conversation

jasoncouture commented Jan 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasoncouture commented Jan 11, 2023

Uh oh!

phil-opp commented Jan 11, 2023

Uh oh!

jasoncouture commented Jan 14, 2023

Uh oh!

phil-opp commented Jan 16, 2023

Uh oh!

phil-opp commented Jan 16, 2023

Uh oh!

jasoncouture commented Jan 17, 2023

Uh oh!

phil-opp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

phil-opp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jasoncouture commented Jan 9, 2023 •

edited

Loading

phil-opp left a comment •

edited

Loading