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 support for a configuration file #326

Merged
merged 10 commits into from
Jan 27, 2023

Conversation

asensio-project
Copy link
Contributor

@asensio-project asensio-project commented Jan 14, 2023

Hello,

This PR adds support for de-serializing a JSON configuration file (config.json). I chose JSON because is the only implementation that works in no-std.

  • Add a configuration file
  • Make it fault tolerant
  • Write the tests
  • Write/Improve docs

Thanks!

common/Cargo.toml Outdated Show resolved Hide resolved
common/src/config.rs Outdated Show resolved Hide resolved
@asensio-project asensio-project marked this pull request as ready for review January 15, 2023 15:50
@jasoncouture
Copy link
Contributor

Should we keep the kernel configuration as is, and use it as defaults if the config doesn't specify? In lieu of removing those options?

@jasoncouture
Copy link
Contributor

Also, shouldnt we load the config first? This would allow the kernel/ramdisk filenames to be set via config if desired (which i personally do desire.)

IE:

  • Set config to defaults.
  • If (config_present) load
  • for settings that are not set, do not set them leave defaults in place.
  • Load kernel, read kernel config.
  • For each setting not present in config file, apply kernel defaults.
  • load ramdisk
  • Continue as normal from this point.

We dont need to add the kernel/ramdisk settings now. My goal in these statements is to avoid a breaking api change when we want to add them, and preserve existing behavior.

We can probably even get away with loading the config after, so long as we keep kernel defaults and make the file optional (if it isn't already). We would then be able to refactor the config file as described above, while sticking to a minor semver bump (where as current seems major to be, since it's a breaking change)

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 a lot!

api/src/config.rs Outdated Show resolved Hide resolved
bios/stage-4/src/main.rs Outdated Show resolved Hide resolved
common/src/config.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/bios/mod.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Outdated Show resolved Hide resolved
uefi/src/main.rs Show resolved Hide resolved
common/src/config.rs Outdated Show resolved Hide resolved
tests/runner/src/lib.rs Outdated Show resolved Hide resolved
@phil-opp
Copy link
Member

@jasoncouture

Should we keep the kernel configuration as is, and use it as defaults if the config doesn't specify? In lieu of removing those options?

We definitely need to keep the frame buffer config in the kernel options for semver compatibility. However, we could mark it as deprecated. Removing the log-related config options is fine because they're not released yet.

Also, shouldnt we load the config first? This would allow the kernel/ramdisk filenames to be set via config if desired (which i personally do desire.)

We can do that, but I don't think that we need to do it in this PR. Loading it earlier shouldn't be a breaking change.

IE:

  • Set config to defaults.
  • If (config_present) load
  • for settings that are not set, do not set them leave defaults in place.
  • Load kernel, read kernel config.
  • For each setting not present in config file, apply kernel defaults.
  • load ramdisk
  • Continue as normal from this point.

I'm not sure if we should make all kernel config options overwritable. The kernel might rely on the config values, e.g. the specific address of a mapping or a minimum stack size. It's probably not a good idea to allow changing these values after compilation.

@asensio-project asensio-project requested review from phil-opp and bjorn3 and removed request for bjorn3 and phil-opp January 22, 2023 09:59
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 a lot for the updates! Looks very good now.

api/src/config.rs Outdated Show resolved Hide resolved
bios/stage-4/src/main.rs Outdated Show resolved Hide resolved
bios/stage-4/src/main.rs Outdated Show resolved Hide resolved
common/Cargo.toml Outdated Show resolved Hide resolved
common/src/config.rs Outdated Show resolved Hide resolved
common/src/config.rs Outdated Show resolved Hide resolved
src/uefi/mod.rs Outdated Show resolved Hide resolved
src/bios/mod.rs Outdated Show resolved Hide resolved
src/bios/mbr.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
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 quick updates!

common/config/src/lib.rs Outdated Show resolved Hide resolved
@phil-opp phil-opp enabled auto-merge (squash) January 27, 2023 13:54
@phil-opp
Copy link
Member

Thanks a lot!

@phil-opp phil-opp merged commit b2d744b into rust-osdev:main Jan 27, 2023
@phil-opp phil-opp mentioned this pull request Mar 12, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants