Some general guidance:

1. Start with a PR that just updatesuefi-raw. (As Philipp notedhere, the definition of the shell protocol that matches the C spec should go in uefi-raw. See for exampleRngProtocol.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 theuefi crate. Small PRs will help get review done faster.
3. RegardingVec, it's generally OK to use, but does need to be gated by thealloc 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 updateuefi-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 theEnv functions and the other forCurDir?

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 theShellProtocol definition moved touefi-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 asuefi-test-runner/src/proto/media.rs anduefi-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 updatedget_envs() to return aVars struct that implements the Iterator trait. However, I do have a question about my implementation. When checking if a givenChar16 isNULL, I use

Char16::from_u16_unchecked(0)

to represent theNULL 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 forChar16. 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 thecstr16! macro instead of the buffers and also added a unit test for theVars struct. Thank you for the tip on mocking the inner value with theVec, 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 returningResult as opposed toStatus, is it alright to simply call theto_result() method and return?

As for changingVars to return both the name and value of the environment variables, is it fine to use something from thealloc crate likeVec orBox 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 avoidalloc whenever possible, but over time,alloc was mandatory for more and more parts of the crate.

Now, IMHO, the guideline is: preventalloc when it is easily doable. Otherwise, go with the convenience ofalloc

Now, IMHO, the guideline is: preventalloc when it is easily doable. Otherwise, go with the convenience ofalloc

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

pubstructVars<'a>{/// Char16 containing names of environment variablesnames:*constChar16,/// Vec containing values of environment variablesvalues:Vec::<Option<&'aCStr16>>,/// Current position in veccur_pos:usize,/// Placeholder to attach a lifetime to `Vars`placeholder:PhantomData<&'aCStr16>,}

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 aVec 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 ofEFI_SHELL_PROTOCOL.GetEnv() whennull is passed in -- does it return just the names of variables, or names and values?

I see your point on storingVec in the iterator. As forEFI_SHELL_PROTOCOL.GetEnv() in the case wherenull is passed in, only the names of the variables are returned. So for returning the values alongside the variables in theVars iterator, it seems that a little extra work has to be done to call and store the results of eitherget_env() orvar() 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.