loader.efi environment related cleanups
ClosedPublic

Authored by tsoome on Jan 12 2017, 11:21 PM.

Details

Summary

Since we have dedicated libefi/env.c file for variable support, the following
changes are done:

Simple cstyle changes in env.c
Moved efi variable related commands from loader/main.c to libefi/env.c
Did create function to set "efi-version" environment variable in env.c.

This function does serve two purposes: for first a  small clean up of the
loader main(), and for second, it does replace the otherwise unused
efi_variable_support hack.

A bit of cleanup of ficl backend functions. The TEST_MAIN has no meaning,
and removed few memory leaks.

The forth code is updated to use "efi-version" variable, instead of ficl
environment check.

Test Plan

Build and boot test. I have not yet done buildworld.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tsoome retitled this revision from to loader.efi environment related cleanups.Jan 12 2017, 11:21 PM
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: imp, allanjude.
imp accepted this revision.Jan 13 2017, 5:10 AM

OK. The code motion is fine. However the UTF conversion is wrong. It mostly works for ASCII / English, but fails for more complicated things. libefivar has the right routines, and it would be good to find a way for libstand to share that with libefivar and use the libstand routines. That can be done in a followup commit though.

sys/boot/efi/libefi/env.c
109 ↗(On Diff #23951)

Does %S due strictly wide characters, or does it do UTF-16 encoded strings? That was one of my cleanup mental notes.

122–125 ↗(On Diff #23951)

I'm not so sure about this heuristic.... I wrote it, but I'm not longer sure it's a reasonable thing. Comments?

sys/boot/efi/libefi/wchar.c
2 ↗(On Diff #23951)

Yea, you didn't write these routines, I did. However, they are wrong. I forgot to go back and do them correctly based on getting libefivar working.

39 ↗(On Diff #23951)

This is wrong. It was wrong before you started though. See what libefivar does, since that's right. Likewise for cpy8to16 and cpy16to8. It's only mostly right :(

This revision is now accepted and ready to land.Jan 13 2017, 5:10 AM
tsoome added inline comments.Jan 13 2017, 7:19 AM
sys/boot/efi/libefi/env.c
109 ↗(On Diff #23951)

%S is just sending CHAR16 string to console as is, assuming the SimpleTextOutput protocol is dealing with it.

122–125 ↗(On Diff #23951)

We actually need to check the names of the variables and interpret based on the description in UEFI specification. For example, the some of them are to be converted to devicepath strings etc.

sys/boot/efi/libefi/wchar.c
2 ↗(On Diff #23951)

Oh yes, I was just moving the functions and did not think too much:D can you please provide proper line for it, so I'll update before going for commit.

tsoome updated this revision to Diff 23963.Jan 13 2017, 1:54 PM

ficlEfiSetenv() already does initialize the string variables

This revision now requires review to proceed.Jan 13 2017, 1:54 PM
tsoome updated this revision to Diff 23964.Jan 13 2017, 2:22 PM

libefi/Makefile needs to build efi.c even without BOOT_FORTH

tsoome updated this revision to Diff 24085.Jan 16 2017, 9:58 PM

fixed the copyright line

smh requested changes to this revision.Jan 23 2017, 9:51 AM
smh added a reviewer: smh.
smh added a subscriber: smh.
smh added inline comments.
sys/boot/efi/libefi/env.c
156 ↗(On Diff #24085)

Alignment issues or just reviews.?

176 ↗(On Diff #24085)

unneeded warp?

209 ↗(On Diff #24085)

missing \n

250 ↗(On Diff #24085)

Can't this check be done earlier in a why that makes it clear how may args are really supported?

272 ↗(On Diff #24085)

switch might make this clearer to read as it clearly associates the EFI_SUCCESS check below with this request.

274 ↗(On Diff #24085)

Do we not have a realloc?

This revision now requires changes to proceed.Jan 23 2017, 9:51 AM
tsoome updated this revision to Diff 24343.Jan 23 2017, 11:11 AM
tsoome marked 2 inline comments as done.

use realloc(), add missing \n.

tsoome added inline comments.Jan 23 2017, 11:12 AM
sys/boot/efi/libefi/env.c
156 ↗(On Diff #24085)

This is phabricator issue with dealing with tabs.

176 ↗(On Diff #24085)

&uuid_status does not fit into 80 chars line.

250 ↗(On Diff #24085)

I'm quite sure it can be, but I would leave it as is for now, as the main purpose for this change is to move the code from efi/loader/main.c. Also there are some additional changes related to variables, so we have to return to this code anyhow, and then it is better time to review the logic here.

272 ↗(On Diff #24085)

This re-allocation also means we need to go for next loop iteration to repeat the GetNextVariableName() with larger buffer, the switch() will complicate the usage of "continue" there.

274 ↗(On Diff #24085)

we do indeed.

sys/boot/efi/libefi/wchar.c
39 ↗(On Diff #23951)

Yep, agree, but I do not want to touch it in this update, leaving those for followups.

tsoome updated this revision to Diff 24348.Jan 23 2017, 1:27 PM

realloc() need to use temporary variable for result check, or
we will get memory leak.

imp accepted this revision.Jan 31 2017, 11:49 PM
This revision was automatically updated to reflect the committed changes.