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

feat(env): parse env=KEY=VALUE for env vars #1394

Merged
merged 4 commits into from
Sep 23, 2024
Merged

feat(env): parse env=KEY=VALUE for env vars #1394

merged 4 commits into from
Sep 23, 2024

Conversation

mkroening
Copy link
Member

@mkroening mkroening commented Sep 20, 2024

This PR

  1. unifies FDT bootargs parsing across all platforms,
  2. adds env=KEY=VALUE kernel arguments for environment variable initialization, and
  3. disables the Uhyve CMDSIZE and CMDVAL hypercalls to use the kernel-maintained environment on Uhyve (breaking).

Questions

  1. Should we use the proposed env=KEY=VALUE format? This differs from our current format for other kernel arguments, such as -freq FREQ and -ip IP. I think we should migrate those to freq=FREQ and ip=IP eventually.
  2. Do we need to support = in environment variable names?
  3. Should we immediately disable the Uhyve hypercalls (easy)? This would break older versions of Uhyve, but I don't think this is a big problem. Alternatively, we could merge the Uhyve hypercall environment with the kernel one (complicated), or we only use the kernel environment when Uhyve returns an empty one (medium).

Extracted 0. commit: #1396
Corresponding Uhyve PR: hermit-os/uhyve#758

Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
@mkroening mkroening marked this pull request as ready for review September 21, 2024 09:23
})
}

pub fn fdt_args() -> Option<&'static str> {
Copy link
Member

Choose a reason for hiding this comment

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

I like brief doc comments on functions, but this is a personal preference.

Shouldn't these functions be pub (crate)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't these functions be pub (crate)?

I prefer not setting the export scope when the whole module is private.


dtb.get_property("/chosen", "bootargs")
.map(|property| str::from_utf8(property).unwrap())
None
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we throw out these functions while we are on it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do away with these architecture-specific bootargs once we have migrated all platforms to FDT bootargs and do the bootinfo version jump. We still have the bootargs in the bootinfo for x86-64 QEMU and Linux boot.

@jounathaen
Copy link
Member

jounathaen commented Sep 22, 2024

LGTM

Should we use the proposed env=KEY=VALUE format? This differs from our current format for other kernel arguments, such as -freq FREQ and -ip IP. I think we should migrate those to freq=FREQ and ip=IP eventually.

Sounds reasonable.

Do we need to support = in environment variables? That would require some tuning of shell_words, I think.

I think this is a must. It is exotic, but would lead to some strange bugs if there is just some environment that uses it.

Should we immediately disable the Uhyve hypercalls (easy)? This would break older versions of Uhyve, but I don't think this is a big problem. Alternatively, we could merge the Uhyve hypercall environment with the kernel one (complicated), or we only use the kernel environment when Uhyve returns an empty one (medium).

Just throw them out. On my V2 of the uhyve interface, these are also not present anymore. I doubt that uhyve is used "in production" right now.

Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
Signed-off-by: Martin Kröning <martin.kroening@eonerc.rwth-aachen.de>
@mkroening
Copy link
Member Author

Do we need to support = in environment variables? That would require some tuning of shell_words, I think.

I think this is a must. It is exotic, but would lead to some strange bugs if there is just some environment that uses it.

I have implemented support for = in environment variable values, but not in names. This is fine according to POSIX.

Just throw them out.

👍

@mkroening mkroening assigned mkroening and unassigned jounathaen Sep 23, 2024
@mkroening mkroening added this pull request to the merge queue Sep 23, 2024
Merged via the queue into main with commit cecfc26 Sep 23, 2024
12 checks passed
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