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

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(a)linux.ibm.com&gt; >> --- >>   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; ?

>>       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?