subr_firmware: implement firmware_path search path
Needs ReviewPublic
Actions

Authored by mcq_disroot.org on Sat, Feb 28, 7:55 PM.

Details

Reviewers

imp
kevans
guest-seuros
manu

Summary

sys/kern/subr_firmware.c had a hardcoded single directory path for
bin firmware loading. this implements the firmware_path sysctl
and kenv variable

the default path is /boot/firmware. the paths can be configured
through loader.conf or through sysctl kern.firmware_path.

Test Plan

tested on a VM so far and didn't break but didn't do much else but set the kern.firmware_path to another directory and was decently stable.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mcq_disroot.org created this revision.Sat, Feb 28, 7:55 PM

Herald added a subscriber: olce. · View Herald TranscriptSat, Feb 28, 7:55 PM

mcq_disroot.org requested review of this revision.Sat, Feb 28, 7:55 PM

mcq_disroot.org added a reviewer: kevans.Sun, Mar 1, 3:18 AM

mcq_disroot.org added a reviewer: guest-seuros.Sun, Mar 1, 5:09 AM

can you either use git arc to update this patch, or generate the diff with -U99999 please? I need a little more context to review it.

Generated with U99999 for full context.

imp added inline comments.Wed, Mar 4, 8:54 PM

sys/kern/subr_firmware.c
272	RWTUN already handles this, so you don't need another TUNABLE_STR here.
354	Do you need to guard against a firmware path that's exactly MAXAPATHLEN bytes long? Do you know that string will be NUL terminated always?
355	strchr() I think would be better here.
367	I'd include the firmware path string here.

A couple of small suggestions, and one question that needs to be answered.

In D55597#1272248, @mcq_disroot.org wrote:

Generated with U99999 for full context.

Thanks! I was able to see the context I couldn't the first time. This looks pretty good.

kevans added inline comments.Wed, Mar 4, 10:16 PM

sys/kern/subr_firmware.c
267	We need to maintain this anyways as a fallback, but I'd probably go ahead and add a default in ^/stand/defaults/loader.conf that matches it -- if nothing else, it's an example

used strchr(3) in try_binary_file() instead of manual parsing
ensured proper checking in try_binary_file(), and added a default for firmware_path in /stand/defaults/loader.conf
also removed TUNABLE_STR because CTLFLAG_RWTUN already sets it

Herald added a reviewer: manu. · View Herald TranscriptWed, Mar 4, 10:58 PM

Herald added a subscriber: dab. · View Herald Transcript

refer to previous commit msg, just added /usr/share/firmware as another path in firmware_path[MAXPATHLEN]