-
Notifications
You must be signed in to change notification settings - Fork 12
SMM is not enabled when TPM is enabled in VMware source client #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pointer.Bool()
and pointer.BoolDeref
7aa4b09
to
614c3f8
Compare
pointer.Bool()
and pointer.BoolDeref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where SMM is not enabled when TPM is enabled on VMware virtual machines, while also standardizing the use of pointer helper functions for safely dereferencing nil pointers. Key changes include:
- Enabling SMM if either secure boot or TPM is enabled in the VMware client.
- Updating test assertions and error messages for consistency.
- Standardizing boolean pointer usage across VMware and OpenStack client implementations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
pkg/source/vmware/client_test.go | Updates test assertions and docker image tag, and adds secureboot tests |
pkg/source/vmware/client.go | Refines firmware, secure boot, TPM, and SMM configuration logic |
pkg/source/openstack/client.go | Standardizes pointer helper usage and similar configuration updates |
Comments suppressed due to low confidence (2)
pkg/source/vmware/client_test.go:208
- [nitpick] Consider using consistent capitalization in error messages (e.g., 'VM export' vs. 'VM CR generation') for clarity and uniformity.
assert.NoError(err, "expected no error during vm export")
pkg/source/vmware/client.go:394
- [nitpick] Consider refactoring the shared firmware and security features configuration (secure boot, TPM, and SMM) into a common helper to reduce duplication with the OpenStack client implementation.
// setup bios/efi, secureboot and tpm settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I have one simple question: Should we also add a unit test for the OpenStack source client?
@starbops Done by extracting the relevant code into a helper function which can be easily tested via unit test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
Related to: harvester/harvester#7984 Signed-off-by: Volker Theile <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
When TPM is enabled, SMM should be enabled as well. The OpenStack client part is doing that already, but the VMware client is missing that.
This PR makes also use of the
pointer.BoolXXX()
helper functions to secure de-referencing of nil pointers.Related to: harvester/harvester#7984