efi: refactoring to prepare for the introduction of efi/preinstall #310
Conversation
efi/preinstall will depend on efi so it is necessary to break some cyclic dependencies, which I've done via the efi/internal package. The most problematic dependency is that efi/preinstall needs to use the efi.PCRProfileOption interface which contains an unexported method that takes a pointer to the unexported pcrProfileGenerator type - a type that should remain unexported. This has been solved by adding a simple internal.PCRProfileOptionVisitor interface that is implemented by pcrProfileGenerator and can be consumed in both efi and efi/preinstall packages. This has simplified direct testing of types that implement the efi.PCRProfileOption interface as well. On this note, this did highlight a couple of testing gaps - we had no direct tests for the WithSignatureDBUpdates and WithShimSbatPolicyLatest options. I've added tests for these, which found a bug in the way the branching for the quirk handling worked for WithSignatureDBUpdates. As part of this, I did rename a couple of types to give them names I felt make more sense. Eg, rootVarsCollector is not variableSetCollector, to bring it more in line with the new internal.VariableSet interface. The rootVarReader type is now initialVarReader, but other than that, no other changes to this code has been made - there's just been a lot of moving of types to the new efi/internal package.
It's not great to mix bug fixes with refactors. Can you point more exactly to the code change part that correspond to the fix as there's only one commit here? |
Unfortunately I wrote the tests as I was developing this and then tested the new tests (slightly adapted to make them build) against the old code out of curiosity, so I didn't exactly isolate the change that fixed it. Given the nature of the failure, I'd imagine it was related to the way the branching was performed (eg, here https://github.com/snapcore/secboot/pull/310/files#diff-77841c9e2208fec4c3f0e3b3f286c19caa5786320d132f2f91b9d0dc57f56a49L61) - there was a missing branch in the results in the case where the test expected each quirk to produce different results. |
efi/preinstallwill depend onefiso it is necessary to break some cyclic dependencies, which I've done via theefi/internalpackage.The most problematic dependency is that
efi/preinstallneeds to use theefi.PCRProfileOptioninterface which contains an unexported method that takes a pointer to the unexportedpcrProfileGeneratortype - a type that should remain unexported. This has been solved by adding a simpleinternal.PCRProfileOptionVisitorinterface that is implemented bypcrProfileGeneratorand can be consumed in bothefiandefi/preinstallpackages. This has simplified direct testing of types that implement theefi.PCRProfileOptioninterface as well.On this note, this did highlight a couple of testing gaps - we had no direct tests for the
WithSignatureDBUpdatesandWithShimSbatPolicyLatestoptions. I've added tests for these, which found a bug in the way the branching for the quirk handling worked forWithSignatureDBUpdates.As part of this, I did rename a couple of types to give them names I felt make more sense. Eg,
rootVarsCollectoris nowvariableSetCollector, to bring it more in line with the newinternal.VariableSetinterface. TherootVarReadertype is nowinitialVarReader, but other than that, no other changes to this code has been made - there's just been a lot of moving of types to the newefi/internalpackage.