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

[Stefan Berger <stefanb@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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?
> Michal