[PATCH 2/2] qemu: tpm: Remove TPM state after successful migration

[Michal Prívozník]

On 10/3/22 21:46, Stefan Berger wrote:
> 
> 
> On 10/3/22 11:20, Michal Prívozník wrote:
>> On 8/23/22 18:28, Stefan Berger wrote:
>>> This patch 'fixes' the behavior of the persistent_state TPM domain XML
>>> attribute that intends to preserve the state of the TPM but should not
>>> keep the state around on all the hosts a VM has been migrated to. It
>>> removes the TPM state directory structure from the source host upon
>>> successful migration when non-shared storage is used. Similarly, it
>>> removes it from the destination host upon migration failure when
>>> non-shared storage is used.
>>>
>>> Signed-off-by: Stefan Berger <stefanb at linux.ibm.com>
>>> ---
>>>   src/qemu/qemu_migration.c | 14 +++++++++++---
>>>   src/qemu/qemu_tpm.h       | 12 ++++++++++++
>>>   2 files changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>>> index 7f69991e2b..355a2cb5c7 100644
>>> --- a/src/qemu/qemu_migration.c
>>> +++ b/src/qemu/qemu_migration.c
>>> @@ -38,6 +38,7 @@
>>>   #include "qemu_security.h"
>>>   #include "qemu_slirp.h"
>>>   #include "qemu_block.h"
>>> +#include "qemu_tpm.h"
>>>     #include "domain_audit.h"
>>>   #include "virlog.h"
>>> @@ -4006,6 +4007,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver,
>>>       qemuMigrationJobPhase phase;
>>>       g_autoptr(virQEMUDriverConfig) cfg =
>>> virQEMUDriverGetConfig(driver);
>>>       qemuDomainObjPrivate *priv = vm->privateData;
>>> +    virDomainUndefineFlagsValues undefFlags = 0;
> 
> unsigned int undefFlags = 0; ?

Given the point you raised in the previous reply, I think you can leave
this as is.

> 
>>>       int ret = -1;
>>>         VIR_DEBUG("vm=%p, flags=0x%x, cancelled=%d", vm, flags,
>>> cancelled);
>>> @@ -4054,7 +4056,9 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver,
>>>               virDomainDeleteConfig(cfg->configDir,
>>> cfg->autostartDir, vm);
>>>               vm->persistent = 0;
>>>           }
>>> -        qemuDomainRemoveInactive(driver, vm, 0);
>>> +        if (!qemuTPMCheckKeepTPMStateMigrationSrcSuccess(vm))
>>> +            undefFlags |= VIR_DOMAIN_UNDEFINE_TPM;
>>> +        qemuDomainRemoveInactive(driver, vm, undefFlags);
>>>       }
>>>      cleanup:
>>> @@ -6590,6 +6594,7 @@ qemuMigrationDstFinishActive(virQEMUDriver
>>> *driver,
>>>       virObjectEvent *event;
>>>       bool inPostCopy = false;
>>>       bool doKill = priv->job.phase !=
>>> QEMU_MIGRATION_PHASE_FINISH_RESUME;
>>> +    virDomainUndefineFlagsValues undefFlags = 0;
>>>       int rc;
>>>         VIR_DEBUG("vm=%p, flags=0x%lx, retcode=%d",
>>> @@ -6666,8 +6671,11 @@ qemuMigrationDstFinishActive(virQEMUDriver
>>> *driver,
>>>                                    jobPriv->migParams,
>>> priv->job.apiFlags);
>>>       }
>>>   -    if (!virDomainObjIsActive(vm))
>>> -        qemuDomainRemoveInactive(driver, vm, 0);
>>> +    if (!virDomainObjIsActive(vm)) {
>>> +        if (!qemuTPMCheckKeepTPMStateMigrationDstFailure(vm))
>>> +            undefFlags |= VIR_DOMAIN_UNDEFINE_TPM;
>>> +        qemuDomainRemoveInactive(driver, vm, undefFlags);
>>> +    }
>>>         virErrorRestore(&orig_err);
>>>       return NULL;
>>> diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h
>>> index f068f3ca5a..c5d8774f07 100644
>>> --- a/src/qemu/qemu_tpm.h
>>> +++ b/src/qemu/qemu_tpm.h
>>> @@ -56,3 +56,15 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver,
>>>                             virCgroup *cgroup)
>>>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
>>>       G_GNUC_WARN_UNUSED_RESULT;
>>> +
>>> +static inline bool
>>> +qemuTPMCheckKeepTPMStateMigrationSrcSuccess(virDomainObj *vm
>>> G_GNUC_UNUSED)
>>> +{
>>> +    return false;
>>> +}
>>> +
>>> +static inline bool
>>> +qemuTPMCheckKeepTPMStateMigrationDstFailure(virDomainObj *vm
>>> G_GNUC_UNUSED)
>>> +{
>>> +    return false;
>>> +}
>>
>> Yeah, I know where you're headed with these two, but G_GNUC_UNUSED
>> breaks syntax-check. And they can be added in shared storage state
>> series.
> 
> But you agree to the expressive names? I am asking because they will
> re-appear then?


Yep, sounds good.

Michal