Add facts to note EFI status #232
Conversation
There was a problem hiding this comment.
@jcpunk Thanks for this! It definitely looks useful. Have a few items that it would be great to get cleaned up if possible.
If you're up for adding tests, you can find examples in spec/unit/facter. If not, that's fine, we can pick it up from there.
If you're OK with me editing this PR a bit, let me know and I would be happy to make the updates and you can re-review before submission.
lib/facter/secure_boot_enabled.rb
Outdated
| File.open(file, 'r') do | hexcode | | ||
| hexcode.read(4) | ||
| code = hexcode.read(16).unpack('H*').first.to_i | ||
| if code == 1 |
There was a problem hiding this comment.
|
I'm perfectly happy to have you make any edits you see fit. As you can probably tell, ruby is not my forte.... |
|
OK I'll do a quick hack-around and you can see if it works for you
|
* Namespaced under `simplib__` to match other new facts * Changed `efi` to `simplib__efi_enabled` to match the intent of the fact * Simplified the `secure_boot_enabled` code and added a touch of error handling
|
@jcpunk See if this does what you expect. |
|
That looks good to me. With the alternate names, it is probably worth getting README.md to match ;) |
|
I'd say this looks perfect :) |
|
Great, going to throw in a quick test and we should be ready to roll |
|
It looks like there are some edge cases with reading the files in @jcpunk Do you have a reference site for the format of these files? Also, this is a loop, is stopping at the first valid match sufficient? |
|
I believe the first match of My initial attempt to read the file was based in part on: but it looks like these[1] are probably the right place to look. I think my provided reads look to be a bit too large... [1] |
|
@jcpunk Ah, spot on with those refs. It looks like we need to actually check two files (for whatever reason) and I do think your reads are a bit aggressive. Would you mind giving it a refactor based on the self-test script? Matching what the devs do should always be right (probably :-D) |
See: tools/testing/selftests/kexec/kexec_common_lib.sh
|
I've reworked it with a note for where the upstream tests are located. The kernel test seems to assume there is just one |
|
@jcpunk Thanks! I've got a case where the |
Also ensure that both files have the correct value as part of the end result
|
@jcpunk Can you see if the latest push works for you? |
|
It returns the right results on my test systems. |
|
Awesome! No way to stuff this through |
* Corrected identified code issues
|
@jcpunk Just waiting for tests to finish before pushing. Thought you might want to take a look at the spec tests that were added for future reference. |
|
:) awesome thanks! |
It can be handy to see which hosts are booted into EFI and which are running secure-boot.