Add UNDEFINE_TPM and UNDEFINE_KEEP_TPM flags to qemuDomainUndefineFlags() API and --tpm and --keep-tpm to 'virsh undefine'. Pass the virDomainUndefineFlagsValues via qemuDomainRemoveInactive() from qemuDomainUndefineFlags() all the way down to qemuTPMEmulatorCleanupHost() and delete TPM storage there considering that the UNDEFINE_TPM flag has priority over the persistent_state attribute from the domain XML. Pass 0 in all other API call sites to qemuDomainRemoveInactive() for now. Signed-off-by: Stefan Berger <stefanb(a)linux.ibm.com&gt; --- include/libvirt/libvirt-domain.h | 6 ++++++ src/qemu/qemu_domain.c | 12 +++++++----- src/qemu/qemu_domain.h | 3 ++- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++----------- src/qemu/qemu_extdevice.c | 5 +++-- src/qemu/qemu_extdevice.h | 3 ++- src/qemu/qemu_migration.c | 12 ++++++------ src/qemu/qemu_process.c | 4 ++-- src/qemu/qemu_snapshot.c | 4 ++-- src/qemu/qemu_tpm.c | 14 ++++++++++---- src/qemu/qemu_tpm.h | 3 ++- tools/virsh-domain.c | 15 +++++++++++++++ 12 files changed, 77 insertions(+), 35 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 7430a08619..5f12c673d6 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2267,9 +2267,15 @@ typedef enum { VIR_DOMAIN_UNDEFINE_CHECKPOINTS_METADATA = (1 << 4), /* If last use of domain, then also remove any checkpoint metadata (Since: 5.6.0) */ + VIR_DOMAIN_UNDEFINE_TPM = (1 << 5), /* Also remove any + TPM state (Since: 8.8.0) */ + VIR_DOMAIN_UNDEFINE_KEEP_TPM = (1 << 6), /* Keep TPM state (Since: 8.8.0) */ + VIR_DOMAIN_UNDEFINE_TPM_MASK = (3 << 5), /* TPM flags mask (Since: 8.8.0) */ + /* Future undefine control flags should come here. */ } virDomainUndefineFlagsValues; + int virDomainUndefineFlags (virDomainPtr domain, unsigned int flags); int virConnectNumOfDefinedDomains (virConnectPtr conn); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index d5fef76211..47eabd0eec 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7143,7 +7143,8 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, static void qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + virDomainUndefineFlagsValues flags) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *snapDir = NULL; @@ -7169,7 +7170,7 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, if (rmdir(chkDir) < 0 && errno != ENOENT) VIR_WARN("unable to remove checkpoint directory %s", chkDir); } - qemuExtDevicesCleanupHost(driver, vm->def); + qemuExtDevicesCleanupHost(driver, vm->def, flags); } @@ -7180,14 +7181,15 @@ qemuDomainRemoveInactiveCommon(virQEMUDriver *driver, */ void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm) + virDomainObj *vm, + virDomainUndefineFlagsValues flags) { if (vm->persistent) { /* Short-circuit, we don't want to remove a persistent domain */ return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, flags); virDomainObjListRemove(driver->domains, vm); } @@ -7209,7 +7211,7 @@ qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, return; } - qemuDomainRemoveInactiveCommon(driver, vm); + qemuDomainRemoveInactiveCommon(driver, vm, 0); virDomainObjListRemoveLocked(driver->domains, vm); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 592ee9805b..6322cfa739 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -683,7 +683,8 @@ int qemuDomainSnapshotDiscardAllMetadata(virQEMUDriver *driver, virDomainObj *vm); void qemuDomainRemoveInactive(virQEMUDriver *driver, - virDomainObj *vm); + virDomainObj *vm, + virDomainUndefineFlagsValues flags); void qemuDomainRemoveInactiveLocked(virQEMUDriver *driver, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 707f4cc1bb..3e0c693db1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1624,7 +1624,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, goto cleanup; if (qemuProcessBeginJob(vm, VIR_DOMAIN_JOB_OPERATION_START, flags) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); goto cleanup; } @@ -1633,7 +1633,7 @@ static virDomainPtr qemuDomainCreateXML(virConnectPtr conn, VIR_NETDEV_VPORT_PROFILE_OP_CREATE, start_flags) < 0) { virDomainAuditStart(vm, "booted", false); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); qemuProcessEndJob(vm); goto cleanup; } @@ -2119,7 +2119,7 @@ qemuDomainDestroyFlags(virDomainPtr dom, ret = 0; endjob: if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); qemuDomainObjEndJob(vm); cleanup: @@ -2738,7 +2738,7 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } qemuDomainObjEndAsyncJob(vm); if (ret == 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); cleanup: virQEMUSaveDataFree(data); @@ -3279,7 +3279,7 @@ qemuDomainCoreDumpWithFormat(virDomainPtr dom, qemuDomainObjEndAsyncJob(vm); if (ret == 0 && flags & VIR_DUMP_CRASH) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); cleanup: virDomainObjEndAPI(&vm); @@ -3591,7 +3591,7 @@ processGuestPanicEvent(virQEMUDriver *driver, endjob: qemuDomainObjEndAsyncJob(vm); if (removeInactive) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } @@ -4069,7 +4069,7 @@ processMonitorEOFEvent(virQEMUDriver *driver, virObjectEventStateQueue(driver->domainEventState, event); endjob: - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); qemuDomainObjEndJob(vm); } @@ -5999,7 +5999,7 @@ qemuDomainRestoreInternal(virConnectPtr conn, virFileWrapperFdFree(wrapperFd); virQEMUSaveDataFree(data); if (vm && ret < 0) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); virDomainObjEndAPI(&vm); return ret; } @@ -6690,7 +6690,7 @@ qemuDomainDefineXMLFlags(virConnectPtr conn, } else { /* Brand new domain. Remove it */ VIR_INFO("Deleting domain '%s'", vm->def->name); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } } @@ -6723,7 +6723,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA | VIR_DOMAIN_UNDEFINE_CHECKPOINTS_METADATA | VIR_DOMAIN_UNDEFINE_NVRAM | - VIR_DOMAIN_UNDEFINE_KEEP_NVRAM, -1); + VIR_DOMAIN_UNDEFINE_KEEP_NVRAM | + VIR_DOMAIN_UNDEFINE_TPM | + VIR_DOMAIN_UNDEFINE_KEEP_TPM, -1); if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM) && (flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) { @@ -6732,6 +6734,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, return -1; } + if ((flags & VIR_DOMAIN_UNDEFINE_TPM) && + (flags & VIR_DOMAIN_UNDEFINE_KEEP_TPM)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cannot both keep and delete TPM")); + return -1; + } + if (!(vm = qemuDomainObjFromDomain(dom))) return -1; @@ -6830,7 +6839,7 @@ qemuDomainUndefineFlags(virDomainPtr dom, */ vm->persistent = 0; if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, flags); ret = 0; endjob: diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index b8e3c1000a..24a57b0f74 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -151,7 +151,8 @@ qemuExtDevicesPrepareHost(virQEMUDriver *driver, void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + virDomainUndefineFlagsValues flags) { size_t i; @@ -159,7 +160,7 @@ qemuExtDevicesCleanupHost(virQEMUDriver *driver, return; for (i = 0; i < def->ntpms; i++) { - qemuExtTPMCleanupHost(def->tpms[i]); + qemuExtTPMCleanupHost(def->tpms[i], flags); } } diff --git a/src/qemu/qemu_extdevice.h b/src/qemu/qemu_extdevice.h index 43d2a4dfff..6b05b59cd6 100644 --- a/src/qemu/qemu_extdevice.h +++ b/src/qemu/qemu_extdevice.h @@ -41,7 +41,8 @@ int qemuExtDevicesPrepareHost(virQEMUDriver *driver, G_GNUC_WARN_UNUSED_RESULT; void qemuExtDevicesCleanupHost(virQEMUDriver *driver, - virDomainDef *def) + virDomainDef *def, + virDomainUndefineFlagsValues flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int qemuExtDevicesStart(virQEMUDriver *driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b3b25d78b4..7f69991e2b 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3408,7 +3408,7 @@ qemuMigrationDstPrepareFresh(virQEMUDriver *driver, * and there is no 'goto cleanup;' in the middle of those */ VIR_FREE(priv->origname); virDomainObjRemoveTransientDef(vm); - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } virDomainObjEndAPI(&vm); virErrorRestore(&origErr); @@ -4054,7 +4054,7 @@ qemuMigrationSrcConfirm(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } cleanup: @@ -6003,7 +6003,7 @@ qemuMigrationSrcPerformJob(virQEMUDriver *driver, virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm); vm->persistent = 0; } - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } virErrorRestore(&orig_err); @@ -6130,7 +6130,7 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); return ret; } @@ -6667,7 +6667,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver, } if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); virErrorRestore(&orig_err); return NULL; @@ -6805,7 +6805,7 @@ qemuMigrationProcessUnattended(virQEMUDriver *driver, qemuMigrationJobFinish(vm); if (!virDomainObjIsActive(vm)) - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 5c8413a6b6..1073fc5ed5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8460,7 +8460,7 @@ qemuProcessAutoDestroy(virDomainObj *dom, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_DESTROYED); - qemuDomainRemoveInactive(driver, dom); + qemuDomainRemoveInactive(driver, dom, 0); qemuDomainObjEndJob(dom); @@ -8926,7 +8926,7 @@ qemuProcessReconnect(void *opaque) if (jobStarted) qemuDomainObjEndJob(obj); if (!virDomainObjIsActive(obj)) - qemuDomainRemoveInactive(driver, obj); + qemuDomainRemoveInactive(driver, obj, 0); virDomainObjEndAPI(&obj); virIdentitySetCurrent(NULL); return; diff --git a/src/qemu/qemu_snapshot.c b/src/qemu/qemu_snapshot.c index 6033deafed..433f3ab2c5 100644 --- a/src/qemu/qemu_snapshot.c +++ b/src/qemu/qemu_snapshot.c @@ -2103,7 +2103,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, } if (qemuSnapshotInternalRevertInactive(driver, vm, snap) < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); return -1; } @@ -2125,7 +2125,7 @@ qemuSnapshotRevertInactive(virDomainObj *vm, start_flags); virDomainAuditStart(vm, "from-snapshot", rc >= 0); if (rc < 0) { - qemuDomainRemoveInactive(driver, vm); + qemuDomainRemoveInactive(driver, vm, 0); return -1; } detail = VIR_DOMAIN_EVENT_STARTED_FROM_SNAPSHOT; diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index d0aed7fa2e..c5e1e39961 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -697,10 +697,15 @@ qemuTPMEmulatorInitPaths(virDomainTPMDef *tpm, * Clean up persistent storage for the swtpm. */ static void -qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm) +qemuTPMEmulatorCleanupHost(virDomainTPMDef *tpm, + virDomainUndefineFlagsValues flags) { - if (!tpm->data.emulator.persistent_state) + if (flags == VIR_DOMAIN_UNDEFINE_TPM) { qemuTPMEmulatorDeleteStorage(tpm); + } else if (!tpm->data.emulator.persistent_state && + (flags & VIR_DOMAIN_UNDEFINE_TPM_MASK) == 0) { + qemuTPMEmulatorDeleteStorage(tpm); + } } @@ -993,9 +998,10 @@ qemuExtTPMPrepareHost(virQEMUDriver *driver, void -qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + virDomainUndefineFlagsValues flags) { - qemuTPMEmulatorCleanupHost(tpm); + qemuTPMEmulatorCleanupHost(tpm, flags); } diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index 9951f025a6..f068f3ca5a 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -35,7 +35,8 @@ int qemuExtTPMPrepareHost(virQEMUDriver *driver, ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -void qemuExtTPMCleanupHost(virDomainTPMDef *tpm) +void qemuExtTPMCleanupHost(virDomainTPMDef *tpm, + virDomainUndefineFlagsValues flags) ATTRIBUTE_NONNULL(1); int qemuExtTPMStart(virQEMUDriver *driver, diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index d2ea4d1c7b..ff9c081d71 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3650,6 +3650,14 @@ static const vshCmdOptDef opts_undefine[] = { .type = VSH_OT_BOOL, .help = N_("keep nvram file") }, + {.name = "tpm", + .type = VSH_OT_BOOL, + .help = N_("remove TPM state") + }, + {.name = "keep-tpm", + .type = VSH_OT_BOOL, + .help = N_("keep TPM state") + }, {.name = NULL} }; @@ -3677,6 +3685,8 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) bool delete_snapshots = vshCommandOptBool(cmd, "delete-storage-volume-snapshots"); bool nvram = vshCommandOptBool(cmd, "nvram"); bool keep_nvram = vshCommandOptBool(cmd, "keep-nvram"); + bool tpm = vshCommandOptBool(cmd, "tpm"); + bool keep_tpm = vshCommandOptBool(cmd, "keep-tpm"); /* Positive if these items exist. */ int has_managed_save = 0; int has_snapshots_metadata = 0; @@ -3702,6 +3712,7 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) VSH_REQUIRE_OPTION("delete-storage-volume-snapshots", "remove-all-storage"); VSH_EXCLUSIVE_OPTIONS("nvram", "keep-nvram"); + VSH_EXCLUSIVE_OPTIONS("tpm", "keep-tpm"); ignore_value(vshCommandOptStringQuiet(ctl, cmd, "storage", &vol_string)); @@ -3729,6 +3740,10 @@ cmdUndefine(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_UNDEFINE_NVRAM; if (keep_nvram) flags |= VIR_DOMAIN_UNDEFINE_KEEP_NVRAM; + if (tpm) + flags |= VIR_DOMAIN_UNDEFINE_TPM; + if (keep_tpm) + flags |= VIR_DOMAIN_UNDEFINE_KEEP_TPM; if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) return false; -- 2.37.1

On 10/3/22 11:20, Michal Prívozník wrote: I will remove it... Should I rename flags to undefine_flags in this patch just so these flags are different from other sets of flags? To make them distinguishable was the primary reason to keep the type around. I'll have a look at this. Yes, right. Stefan

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; 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; +} -- 2.37.1

On 8/23/22 12:54, Stefan Berger wrote: These function names are obviously quite expressive but also need to be so that one knows what the reason is for having them called... Any way to proceed on this series? Stefan

On 10/3/22 11:20, Michal Prívozník wrote: unsigned int undefFlags = 0; ? But you agree to the expressive names? I am asking because they will re-appear then?