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.)

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.

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.

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.

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.

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.

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.

LGTM! Please squash your commits a little to smaller but sensible units, then I think we are good to go

Commits have been squashed :)

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 and will do my best to keep this in mind for future pull requests! I've addressed the function call names and return values.

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

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? I don't think I see a way to handle this without dynamically allocating memory to, at the very least, hold the pointers to the values. Please let me know if you have any suggested approaches.

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

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>,
}

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?

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.

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?

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.

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.

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.

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!