On Tue, Aug 02, 2022 at 09:03:01AM +0200, Michal Prívozník wrote:
> On 8/1/22 18:10, Andrea Bolognani wrote:
> > On Fri, Jul 29, 2022 at 09:42:13AM +0200, Michal Privoznik wrote:
> >> +++ b/tests/testutilsqemu.c
> >> @@ -150,12 +150,13 @@ bool
> >> virTPMSwtpmSetupCapsGet(virTPMSwtpmSetupFeature cap)
> >> {
> >> switch (cap) {
> >> + case VIR_TPM_SWTPM_SETUP_FEATURE_TPM_1_2:
> >> + case VIR_TPM_SWTPM_SETUP_FEATURE_TPM_2_0:
> >> + return true;
> >> case VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_PWDFILE_FD:
> >> case VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_CREATE_CONFIG_FILES:
> >> case VIR_TPM_SWTPM_SETUP_FEATURE_TPM12_NOT_NEED_ROOT:
> >> case VIR_TPM_SWTPM_SETUP_FEATURE_CMDARG_RECONFIGURE_PCR_BANKS:
> >> - case VIR_TPM_SWTPM_SETUP_FEATURE_TPM_1_2:
> >> - case VIR_TPM_SWTPM_SETUP_FEATURE_TPM_2_0:
> >> case VIR_TPM_SWTPM_SETUP_FEATURE_LAST:
> >> break;
> >> }
> >
> > So our test suite will work against a mocked TPM that supports a very
> > small set of hardcoded capabilities. Would it make sense to extend
> > this so that it's possible to control things as the test case level,
> > so that we can have coverage for things like e.g. trying to use TPM
> > 1.2 when the swtpm binary only supports TPM 2.0?
>
> Good point. We could have an environment variable that, if set, would
> make this function return just a subset of TPM versions. And then, when
> we want to test scenario you suggest we could do the following:
>
> setenv("VIR_TEST_MOCK_FAKE_TPM_VERSION", "1.2");
> DO_TEST_FAILURE("guest-tpm-2.0");
> unsetenv("VIR_TEST_MOCK_FAKE_TPM_VERSION");
>
> Alternatively, we can modify the DO_TEST_* macro so that it accepts TPM
> version and sets the ENV var accordingly, but I'd rather not do that,
> because it's not very extensible.
That covers the basic scenario of mocking a build of swtpm that has
only one TPM version enabled, but for example until very recently
many builds supported both versions. And no other recent / optional
feature can be toggled this way.
Without thinking too much about it or prototyping it, having
DO_TEST_FULL() accept a new ARG_SWTPMSETUP_FEATURE argument that
contains a list of virTPMSwtpmSetupFeature sounds like a reasonable
enough approach. The more commonly used macros would of course not
expose this argument.
> > That'd all be follow-up work, of course. Your change is good to have
> > as is :)
>
> In fact, I can just not merge this patch and send v2 of this patch that
> implements the idea from above. I'll push the other two though, because
> they fix the issue.
This one's already nice and the discussion on how to make it more
generic could end up taking a bit. I'd say just push it as is now,
then work on improving upon it later.
--
Andrea Bolognani / Red Hat / Virtualization
