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

kargs: Various cleanups #645

Merged
merged 10 commits into from
Jun 28, 2024
Merged

kargs: Various cleanups #645

merged 10 commits into from
Jun 28, 2024

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Jun 27, 2024

No functional changes intended; just prep for further work.


kargs: parser cleanups

  • Accept &str vs owned String, allowing dropping unnecessary clone
  • It isn't parsing a file necessarily, just a string buffer, so rename
    it
  • Drop unnecessary pub
  • Add doc comment
  • Add error context to callers to be clear which file we failed
    to parse

Signed-off-by: Colin Walters [email protected]


kargs: Add a few more unit tests

Signed-off-by: Colin Walters [email protected]


kargs: Parse booted kargs via cap-std, not liboverdrop

liboverdrop isn't helpful here because I don't think we want
or need right now to actually support things like having a file
in /etc or /run override the kargs. Especially for /run
there's no dynamic runtime state.

No functional changes intended, just prep for more work.


It might in theory be useful for someone to have /etc
overrides, but since we weren't actually doing that today, don't
pretend we might by using liboverdrop but not passing /etc to
it.

Using cap-std here makes it easier to unit test safely.
(Although, liboverdrop really should grow an optional feature
to use cap-std)

Signed-off-by: Colin Walters [email protected]


kargs: Drop unnecessary cloning for sys_arch

Since the function accepts a &str, drop the unnecessary cloning.

Signed-off-by: Colin Walters [email protected]


kargs: Drop unnecessary mut

By passing ownership of the vector here we don't need to make
it mutable.

Signed-off-by: Colin Walters [email protected]


kargs: Drop unnecessary allocation and mut

We can just pass the iterator directly to extend.

Signed-off-by: Colin Walters [email protected]


kargs: Minor code reordering

No functional changes, but it's clearer to have the remote_kargs
definition below where we do the short-circuit.

Signed-off-by: Colin Walters [email protected]


- Accept &str vs owned String, allowing dropping unnecessary `clone`
- It isn't parsing a file necessarily, just a string buffer, so rename
  it
- Drop unnecessary `pub`
- Add doc comment
- Add error context to callers to be clear *which* file we failed
  to parse

Signed-off-by: Colin Walters <[email protected]>
liboverdrop isn't helpful here because I don't think we want
or need right now to actually support things like having a file
in `/etc` or `/run` override the kargs. Especially for `/run`
there's no dynamic runtime state.

It *might* in theory be useful for someone to have `/etc`
overrides, but since we weren't actually doing that today, don't
pretend we might by using liboverdrop but not passing `/etc` to
it.

Using cap-std here makes it easier to unit test safely.
(Although, liboverdrop really should grow an optional feature
 to use cap-std)

Signed-off-by: Colin Walters <[email protected]>
Since the function accepts a `&str`, drop the unnecessary cloning.

Signed-off-by: Colin Walters <[email protected]>
By passing ownership of the vector here we don't need to make
it `mut`able.

Signed-off-by: Colin Walters <[email protected]>
We can just pass the iterator directly to `extend`.

Signed-off-by: Colin Walters <[email protected]>
No functional changes, but it's clearer to have the `remote_kargs`
definition below where we do the short-circuit.

Signed-off-by: Colin Walters <[email protected]>
- Change filtering to use combinators which is a bit clearer
  I think
- Rework to directly return the deserialized args from the toml
  file and avoid moving them into a new vector
- Drop the now unnecessary `mut`

Signed-off-by: Colin Walters <[email protected]>
Copy link
Contributor

@lukewarmtemp lukewarmtemp left a comment

Choose a reason for hiding this comment

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

LGTM

Prep for more tests.

Signed-off-by: Colin Walters <[email protected]>
Somehow, something else is leaking into `/tmp` at least in the
GHA runs...remove it all.

Signed-off-by: Colin Walters <[email protected]>
@cgwalters cgwalters merged commit 0bd1956 into containers:main Jun 28, 2024
16 of 19 checks passed
cgwalters pushed a commit to cgwalters/bootc that referenced this pull request Nov 5, 2024
…-0.4.38

build(deps): bump chrono from 0.4.37 to 0.4.38
cgwalters added a commit to cgwalters/bootc that referenced this pull request Nov 6, 2024
Fix deprecation warning with rust 1.79.0
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.

2 participants