Is there anything I can do here?

Updated to remove the extra newline

Should note I just got done testing this with a clean 14.1 src tree that I applied the git diff to with no other changes; tested sha256 password and sha512 password, additionally un-commented both test vector suites and all passed.

In D41509#1055376, @david_crossfamilyweb.com wrote:

Updated the diff per requested copyright changes.

My intent was that I had implemented all of kevans' reivew changes already, lua-lint passes cleanly-ish. the only issues are about the 'hash' accesses, but those mirror 'gfx' in other lua files.

Updated the diff per requested copyright changes.

Thanks for the rebase. I have one minor housekeeping issue here, but otherwise defer to kevans' reviews (I can't tell if they are folded in or not, but they don't appear to be).

Synced with latest freebsd/main for diff generation, no other changes

As promised, updated, including modifications to export sha512 from C, updated lua crypt implementation to use exported C routines, removed the now unneeded pure lua implementation, I ran all validation suites on my computer and this is passing.

In D41509#1055136, @imp wrote:

Great! I'd forgotten I'd only included it in kboot since that's the only place I'd tested it.
I'll include it elsewhere by default. IIRC, the lua glue code is only about 200 extra bytes of code, so we can have it in the BIOS loader. We're not that close to the edge on it. Normally, I'd say no new features there, but this is a reasonable exception. The big ones that will be hard in the future is there will be no new ZFS compression or crypto supported there.

Great! I'd forgotten I'd only included it in kboot since that's the only place I'd tested it.
I'll include it elsewhere by default. IIRC, the lua glue code is only about 200 extra bytes of code, so we can have it in the BIOS loader. We're not that close to the edge on it. Normally, I'd say no new features there, but this is a reasonable exception. The big ones that will be hard in the future is there will be no new ZFS compression or crypto supported there.

In D41509#1054842, @david_crossfamilyweb.com wrote:

In D41509#1054254, @imp wrote:

In D41509#1054253, @david_crossfamilyweb.com wrote:

I can work on that and validate it; checking quickly it looks like the version exported is compatible (the big issue with other implementations was being given the binary form of the hash to work with, your export seems to do that).

However only sha256 seems to be available there; sha512 is the default for password hashing. I only had done the sha256 since the structure was so similar to sha512 in both the hash mechanism and crypt(3) mechanism that it was easy, but SHA512 was the primary target.

I can add it easily enough. I just didn't need it for the thing I was working on.

Could I get a quick pointer as to accessing these in the code?

I've tried 'require("hash")' and 'loader.sha256' and even 'loader.hash.sha256') (tried modeling these after the gfx module which seems at a high level to be implemented the same way: LUA_COMPILE_SET() to an _init function that luaL_require the module,and then accessing it via the loader hash, but no matter what I try I get a nil dereferene

In D41509#1054254, @imp wrote:

In D41509#1054253, @david_crossfamilyweb.com wrote:

I can work on that and validate it; checking quickly it looks like the version exported is compatible (the big issue with other implementations was being given the binary form of the hash to work with, your export seems to do that).

However only sha256 seems to be available there; sha512 is the default for password hashing. I only had done the sha256 since the structure was so similar to sha512 in both the hash mechanism and crypt(3) mechanism that it was easy, but SHA512 was the primary target.

I can add it easily enough. I just didn't need it for the thing I was working on.

In D41509#1054254, @imp wrote:

In D41509#1054253, @david_crossfamilyweb.com wrote:

I can work on that and validate it; checking quickly it looks like the version exported is compatible (the big issue with other implementations was being given the binary form of the hash to work with, your export seems to do that).

However only sha256 seems to be available there; sha512 is the default for password hashing. I only had done the sha256 since the structure was so similar to sha512 in both the hash mechanism and crypt(3) mechanism that it was easy, but SHA512 was the primary target.

I can add it easily enough. I just didn't need it for the thing I was working on.

In D41509#1054253, @david_crossfamilyweb.com wrote:

I can work on that and validate it; checking quickly it looks like the version exported is compatible (the big issue with other implementations was being given the binary form of the hash to work with, your export seems to do that).

However only sha256 seems to be available there; sha512 is the default for password hashing. I only had done the sha256 since the structure was so similar to sha512 in both the hash mechanism and crypt(3) mechanism that it was easy, but SHA512 was the primary target.

I can work on that and validate it; checking quickly it looks like the version exported is compatible (the big issue with other implementations was being given the binary form of the hash to work with, your export seems to do that).

I added lua bindings for the Sha hash functions. Can those be used instead of the lua implementation?

updated diff -U 99999

Could this be imported as is, and then when the hash modules are in and exported it could be switched over?

In D41509#1022312, @imp wrote:

I've created a hash module that has some of the sha routines. I don't have big issues connecting those.

I've created a hash module that has some of the sha routines. I don't have big issues connecting those.

Updated with all PR comments, save 1.

src/sys/contrib/dev/acpica/include/actypes.h:235:41: error: redefinition of typedef 'BOOLEAN' is a C11 feature [-Werror,-Wtypedef-redefinition]
typedef unsigned char BOOLEAN;

^

/usr/src/stand/efi/include/efidef.h:33:25: note: previous definition is here
typedef UINT8 BOOLEAN;

This broke the armv7 build...
https://ci.freebsd.org/job/FreeBSD-main-armv7-build/19710/
Please fix ASAP.

I'm not seeing any remaining problems here, seems to LGTM.

Just one minor nit

looks good now. Thanks for the split.

Added @jrtc27 and @mhorne to chime in concerning @imp's question concerning risc-v (and possibly armv7)

In D42459#969394, @imp wrote:

I generally like this change, one or two quibbles to work out.
I'd also split the lua and other stuff into separate commits (the old lua code will work with the new loader.efi, which I like as well).
But I can do the splitting if that's a hassle. This is otherwise fairly clean so I wouldn't mind a small amount of extra work.

Stripped out lua diff to submit separately

I generally like this change, one or two quibbles to work out.
I'd also split the lua and other stuff into separate commits (the old lua code will work with the new loader.efi, which I like as well).
But I can do the splitting if that's a hassle. This is otherwise fairly clean so I wouldn't mind a small amount of extra work.

Address Warner's comment regarding legacy hints and update the man page as suggested by Kyle

I'll take a look at the rest of this later...

Nothing too concerning, mostly some style complaints but as a general rule I'd like to keep luacheck happy for the times it does actually identify legitimate issues.

6. Updated documentation