efi shell interface protocol: add var(), vars(), and set_var()

[RenTrieu]

This PR implements var(), vars(), and set_var() for the efi shell interface protocol.

It is part of #448 where @timrobertsdev was laying the groundwork (thanks!)!

Covered EFI Shell Intercace Protocol Functions

• GetEnv
• SetEnv

[phip1611]

Thanks for working on this! As a general rule of thumb: High-level wrappers should be close to the API of std and not replicate "ugly" or unusual details of the low-level API. See my suggestions for get_var

[nicholasbishop]

Some general guidance:

1. Start with a PR that just updates uefi-raw. (As Philipp noted here, the definition of the shell protocol that matches the C spec should go in uefi-raw. See for example RngProtocol. api_guidelines has some notes about how to implement stuff in uefi-raw.)
2. Once that's merged, do small PRs that add ergonomic wrappers to the uefi crate. Small PRs will help get review done faster.
3. Regarding Vec, it's generally OK to use, but does need to be gated by the alloc feature. (I haven't looked at the code yet though, I'm just commenting generally.)

[RenTrieu]

Thanks for the feedback! I will start with making the PR to update uefi-raw as Nicholas suggested, then come back to address the other comments.

[RenTrieu]

Hi all. Now that #1680 is merged, I was wondering if it would be alright to use this PR to work on the 4 ergonomic wrappers I've implemented (GetEnv(), SetEnv(), GetCurDir(), SetCurDir()). Is this sufficiently small enough? Or should I split this into two PRs- one for the Env functions and the other for CurDir?

If it is okay to continue to use this PR, then I plan to rebase this branch onto the current main, squash the commits, and then continue from there.

[nicholasbishop]

That sounds good, start with doing that in this PR. Once the changes are in this PR it's possible I might ask for it to be split up into more than one PR for review, but we can start with the assumption that one PR is OK.

[RenTrieu]

I've rebased this branch onto the current main and updated it to disclude the ShellProtocol definition moved to uefi-raw. I was wondering if there is any guidance for writing tests. So far, I have done my best to look at other files and imitate the format such as uefi-test-runner/src/proto/media.rs and uefi-test-runner/src/proto/rng.rs, but if there is anything more specific I can revise what I have.

[RenTrieu]

Thanks for the feedback! I have updated get_envs() to return a Vars struct that implements the Iterator trait. However, I do have a question about my implementation. When checking if a given Char16 is NULL, I use

Char16::from_u16_unchecked(0)

to represent the NULL value for the comparison. I saw that one of the suggestions requested to use

Char16::from_u16(0).unwrap()

instead, but I don't see this function implemented for Char16. Is it alright to use the unchecked variant?

I'll start working on unit tests next! However, for Miri I will need to take some time to read its docs since I'm not familiar.

[RenTrieu]

I've updated the PR to use the cstr16! macro instead of the buffers and also added a unit test for the Vars struct. Thank you for the tip on mocking the inner value with the Vec, it was very helpful. Please let me know if the unit test looks alright.

[phip1611]

Thank you for the feedback. I'm starting to understand the goal of abstracting the lower level implementation in ways that are useful for the user

Perfect! Thanks for your patience and valuable contributions :)

For returning Result as opposed to Status, is it alright to simply call the to_result() method and return?

Yes, perfectly fine, and also done in other parts of the code base

As for changing Vars to return both the name and value of the environment variables, is it fine to use something from the alloc crate like Vec or Box for this

Yes, that's fine. So far, we didn't have a clear guidline. IMHO, uefi in the early days tried to avoid alloc whenever possible, but over time, alloc was mandatory for more and more parts of the crate.

Now, IMHO, the guideline is: prevent alloc when it is easily doable. Otherwise, go with the convenience of alloc

[RenTrieu]

Now, IMHO, the guideline is: prevent alloc when it is easily doable. Otherwise, go with the convenience of alloc

In this case, would be it be okay to simply store the vector of values as one of the fields of Var? I was thinking of something like:

pub struct Vars<'a> {
    /// Char16 containing names of environment variables
    names: *const Char16,
    /// Vec containing values of environment variables
    values: Vec::<Option<&'a CStr16>>,
    /// Current position in vec
    cur_pos: usize,
    /// Placeholder to attach a lifetime to `Vars`
    placeholder: PhantomData<&'a CStr16>,
}

[nicholasbishop]

In this case, would be it be okay to simply store the vector of values as one of the fields of Var?

Ideally we wouldn't store a Vec of all env values (this somewhat defeats the purpose of it being an iterator, since it requires extra allocations). I'm a little unclear on the behavior of EFI_SHELL_PROTOCOL.GetEnv() when null is passed in -- does it return just the names of variables, or names and values?

[RenTrieu]

I see your point on storing Vec in the iterator. As for EFI_SHELL_PROTOCOL.GetEnv() in the case where null is passed in, only the names of the variables are returned. So for returning the values alongside the variables in the Vars iterator, it seems that a little extra work has to be done to call and store the results of either get_env() or var() for each variable name.

[nicholasbishop]

Could Vars store a reference to the protocol, and call get_env in the next function to get each variable value one at a time?

[RenTrieu]

That's quite a clever approach! I've written an initial implementation and did not find any issues. I will push it shortly so we can review it.

[nicholasbishop]

Hi, just getting back to reviewing this. Could you create a new PR with the current_dir/set_current_dir changes, so that this one just has var/vars/set_var? Getting the per-PR changes smaller will help me review it more quickly.

[RenTrieu]

Sure thing. I'll need a bit to read through and separate the changes, but as soon as I do I'll push the changes and open the new PR.

[RenTrieu]

I've separated the CurDir functions into #1740 and removed their respective changes from this PR. Please let me know if there's anything I've missed!

[phip1611]

Hey @RenTrieu I know this is taking quite long, but we are doing very good and approaching the finishing line! Could you please rebase your branch? Then we can talk about a pragmatic way forward to merge more functionality

[RenTrieu]

@phip1611 I've rebased my branch. Please let me know if there are still any improvements you think we can make. By the way, as long as it is okay with you and Nicholas, I don't mind the pace that the PR process has been going at. Should we discuss the approach to future functionality in this thread or would #448 be more appropriate?

[phip1611]

I've updated the PR title and description for better clarity. In the future, please try to keep them concise, precise, and up to date.

[phip1611]

LGTM, we’re almost there! Just a few more changes. Also, please squash your commits into meaningful units - this makes reviews easier, as Nicholas and I prefer to review commit-by-commit. Having multiple “init A; refactor A; refactor A again” commits are hard to follow.

[RenTrieu]

Hi Philipp, thank you for your review. I've addressed the nits and attempted to squash the commits into meaningful ones. Would you mind taking a look at the squashes I've chosen? I can see how it would be confusing to go through multiple refactor commits. Given that the original commit history looked like "init A; refactor A; refactor A again", is it sufficient to squash all of the refactor commits into one such that the commit history should now look like "init A; refactor A"?

[phip1611]

Hi Philipp, thank you for your review. I've addressed the nits and attempted to squash the commits into meaningful ones. Would you mind taking a look at the squashes I've chosen? I can see how it would be confusing to go through multiple refactor commits. Given that the original commit history looked like "init A; refactor A; refactor A again", is it sufficient to squash all of the refactor commits into one such that the commit history should now look like "init A; refactor A"?

In your case, I'd squash commit C into A. Then you'll end up with:

• uefi: implement var(), vars() and set_var() for shell interface proto
• uefi-test-runner: add integration tests for shell interface proto

[phip1611]

I think this can be simplified to

Suggested change

	vars_mock.push(b'f' as u16);
	vars_mock.extend_from_slice(b"foo1\0bar\0baz2\0\0");

[phip1611]

At least

Suggested change

	fn new(names: Vec<&'a CStr16>, values: Vec<&'a CStr16>) -> ShellMock<'a> {
	fn new(names: &[&'a CStr16], values: &[&'a CStr16]) -> ShellMock<'a> {

you should also assert that both's len() equals.

But much better would be:

Suggested change

	fn new(names: Vec<&'a CStr16>, values: Vec<&'a CStr16>) -> ShellMock<'a> {
	fn new(pairs: impl IntoIterator<Item = (&CStr16, &CStr16)>) -> ShellMock<'a> {

as this is your unit test where you have control over. Your initial strategy is way too complicated; it can be done much simpler :)

[phip1611]

I'll will finally approve this once the open problems are addressed. Thanks for your patience and learning and improving your skills along the way :)