[PATCH 0/3] qemu_tpm: Do not pollute logs with unnecessary warning
https://issues.redhat.com/browse/RHEL-80155 Martin Kletzander (3): qemu_tpm: Rename qemuTPMHasSharedStorage -> qemuTPMDomainHasSharedStorage qemu_tpm: Extract per-TPM functionality from qemuTPMDomainHasSharedStorage qemu_tpm: Only warn about missing locking feature on shared filesystems src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++---------------- src/qemu/qemu_tpm.h | 4 +-- 3 files changed, 47 insertions(+), 34 deletions(-) -- 2.50.1
From: Martin Kletzander <mkletzan@redhat.com> The function deals with the whole domain and the part that handles one TPM will be useful elsewhere and hence extracted later. This rename makes it possible for the new function to use the original name of this renamed one. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_tpm.c | 8 ++++---- src/qemu/qemu_tpm.h | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index c8974dbc5b97..b22248e3b92f 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1721,7 +1721,7 @@ qemuMigrationSrcIsAllowed(virDomainObj *vm, } } - if (qemuTPMHasSharedStorage(driver, vm->def) && + if (qemuTPMDomainHasSharedStorage(driver, vm->def) && !qemuTPMCanMigrateSharedStorage(vm->def)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("the running swtpm does not support migration with shared storage")); diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index b2f76e6b8b31..8c104ab1b303 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -1150,7 +1150,7 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, virCommandSetPidFile(cmd, pidfile); virCommandSetErrorFD(cmd, &errfd); - if (incomingMigration && qemuTPMHasSharedStorage(driver, vm->def)) { + if (incomingMigration && qemuTPMDomainHasSharedStorage(driver, vm->def)) { /* If the TPM is being migrated over shared storage, we can't * lock all files before labeling them: the source swtpm * process is still holding on to the lock file, and it will @@ -1219,8 +1219,8 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, bool -qemuTPMHasSharedStorage(virQEMUDriver *driver, - virDomainDef *def) +qemuTPMDomainHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); size_t i; @@ -1346,7 +1346,7 @@ qemuExtTPMStop(virQEMUDriver *driver, return; qemuTPMEmulatorStop(cfg->swtpmStateDir, shortName); - if (migration && qemuTPMHasSharedStorage(driver, vm->def)) + if (migration && qemuTPMDomainHasSharedStorage(driver, vm->def)) restoreTPMStateLabel = false; if (qemuSecurityRestoreTPMLabels(driver, vm, restoreTPMStateLabel, false) < 0) diff --git a/src/qemu/qemu_tpm.h b/src/qemu/qemu_tpm.h index f0f16392a165..2d633fe36b84 100644 --- a/src/qemu/qemu_tpm.h +++ b/src/qemu/qemu_tpm.h @@ -61,8 +61,8 @@ int qemuExtTPMSetupCgroup(virQEMUDriver *driver, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -bool qemuTPMHasSharedStorage(virQEMUDriver *driver, - virDomainDef *def) +bool qemuTPMDomainHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -- 2.50.1
From: Martin Kletzander <mkletzan@redhat.com> This way we can do the check for a particular TPM also elsewhere in the code, especially in places where we're dealing with only one TPM. The semantics is changed a little bit in a way that the function will check all the TPMs as opposed to stopping on the first one which is of the emulator type, but since a domain can currently only have one of these it was not an issue. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_tpm.c | 59 ++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 8c104ab1b303..855d732e60d0 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -205,6 +205,40 @@ qemuTPMEmulatorCreateStorage(virDomainTPMDef *tpm, } +static bool +qemuTPMHasSharedStorage(const virQEMUDriverConfig *cfg, + const virDomainTPMDef *tpm) +{ + switch (tpm->type) { + case VIR_DOMAIN_TPM_TYPE_EMULATOR: + return virFileIsSharedFS(tpm->data.emulator.source_path, + cfg->sharedFilesystems) == 1; + case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: + case VIR_DOMAIN_TPM_TYPE_EXTERNAL: + case VIR_DOMAIN_TPM_TYPE_LAST: + break; + } + + return false; +} + + +bool +qemuTPMDomainHasSharedStorage(virQEMUDriver *driver, + virDomainDef *def) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + size_t i; + + for (i = 0; i < def->ntpms; i++) { + if (qemuTPMHasSharedStorage(cfg, def->tpms[i])) + return true; + } + + return false; +} + + /** * qemuTPMEmulatorDeleteStorage: * @tpm: TPM definition @@ -1218,31 +1252,6 @@ qemuTPMEmulatorStart(virQEMUDriver *driver, } -bool -qemuTPMDomainHasSharedStorage(virQEMUDriver *driver, - virDomainDef *def) -{ - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - size_t i; - - for (i = 0; i < def->ntpms; i++) { - virDomainTPMDef *tpm = def->tpms[i]; - - switch (tpm->type) { - case VIR_DOMAIN_TPM_TYPE_EMULATOR: - return virFileIsSharedFS(tpm->data.emulator.source_path, - cfg->sharedFilesystems) == 1; - case VIR_DOMAIN_TPM_TYPE_PASSTHROUGH: - case VIR_DOMAIN_TPM_TYPE_EXTERNAL: - case VIR_DOMAIN_TPM_TYPE_LAST: - break; - } - } - - return false; -} - - bool qemuTPMCanMigrateSharedStorage(virDomainDef *def) { -- 2.50.1
From: Martin Kletzander <mkletzan@redhat.com> The warning pollutes the logs and might give a bad impression on someone reading them even though the locking is not always needed. This way we at least limit the logging in unnecessary cases. Resolves: https://issues.redhat.com/browse/RHEL-80155 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_tpm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 855d732e60d0..cdbd6e3993b2 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd, static void qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, - const virDomainTPMEmulatorDef *emulator) + const virDomainTPMEmulatorDef *emulator, + const virDomainTPMDef *tpmDef, + const virQEMUDriverConfig *cfg) { const char *lock = ",lock"; if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { - VIR_WARN("This swtpm version doesn't support explicit locking"); + if (qemuTPMHasSharedStorage(cfg, tpmDef)) + VIR_WARN("This swtpm version doesn't support explicit locking"); + lock = ""; } @@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef *emulator, virCommandAddArgList(cmd, "socket", "--print-info", "0x20", "--tpm2", NULL); - qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg); if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0) return -1; @@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path); - qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator, persistentTPMDef, cfg); virCommandAddArg(cmd, "--log"); if (tpm->data.emulator.debug != 0) -- 2.50.1
On a Thursday in 2025, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
The warning pollutes the logs and might give a bad impression on someone reading them even though the locking is not always needed. This way we at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_tpm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 855d732e60d0..cdbd6e3993b2 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, - const virDomainTPMEmulatorDef *emulator) + const virDomainTPMEmulatorDef *emulator, + const virDomainTPMDef *tpmDef, + const virQEMUDriverConfig *cfg)
Interesting decision that we need to pass the QEMU driver config just to check for an error...
{ const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) {
... but nothing to get the capability. Jano
- VIR_WARN("This swtpm version doesn't support explicit locking"); + if (qemuTPMHasSharedStorage(cfg, tpmDef)) + VIR_WARN("This swtpm version doesn't support explicit locking"); + lock = ""; }
On Thu, Jul 17, 2025 at 12:34:43PM +0200, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
The warning pollutes the logs and might give a bad impression on someone reading them even though the locking is not always needed. This way we at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_tpm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 855d732e60d0..cdbd6e3993b2 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, - const virDomainTPMEmulatorDef *emulator) + const virDomainTPMEmulatorDef *emulator, + const virDomainTPMDef *tpmDef, + const virQEMUDriverConfig *cfg) { const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { - VIR_WARN("This swtpm version doesn't support explicit locking"); + if (qemuTPMHasSharedStorage(cfg, tpmDef)) + VIR_WARN("This swtpm version doesn't support explicit locking"); + lock = ""; }
@@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef *emulator,
virCommandAddArgList(cmd, "socket", "--print-info", "0x20", "--tpm2", NULL);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg);
if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0) return -1; @@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator, persistentTPMDef, cfg);
Coverity complains here that the `persistentTPMDef` can be NULL and that it is dereferenced in `qemuTPMHasSharedStorage`. This is called from `qemuExtDevicesStart` where the for loop calling `qemuExtTPMStart` can actually pass NULL. Not sure if it can happen with any real VM. Pavel
virCommandAddArg(cmd, "--log"); if (tpm->data.emulator.debug != 0) -- 2.50.1
On Fri, Jul 18, 2025 at 11:03:56AM +0200, Pavel Hrdina wrote:
On Thu, Jul 17, 2025 at 12:34:43PM +0200, Martin Kletzander via Devel wrote:
From: Martin Kletzander <mkletzan@redhat.com>
The warning pollutes the logs and might give a bad impression on someone reading them even though the locking is not always needed. This way we at least limit the logging in unnecessary cases.
Resolves: https://issues.redhat.com/browse/RHEL-80155 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/qemu/qemu_tpm.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_tpm.c b/src/qemu/qemu_tpm.c index 855d732e60d0..cdbd6e3993b2 100644 --- a/src/qemu/qemu_tpm.c +++ b/src/qemu/qemu_tpm.c @@ -660,12 +660,16 @@ qemuTPMVirCommandSwtpmAddEncryption(virCommand *cmd,
static void qemuTPMVirCommandSwtpmAddTPMState(virCommand *cmd, - const virDomainTPMEmulatorDef *emulator) + const virDomainTPMEmulatorDef *emulator, + const virDomainTPMDef *tpmDef, + const virQEMUDriverConfig *cfg) { const char *lock = ",lock";
if (!virTPMSwtpmCapsGet(VIR_TPM_SWTPM_FEATURE_TPMSTATE_OPT_LOCK)) { - VIR_WARN("This swtpm version doesn't support explicit locking"); + if (qemuTPMHasSharedStorage(cfg, tpmDef)) + VIR_WARN("This swtpm version doesn't support explicit locking"); + lock = ""; }
@@ -721,7 +725,7 @@ qemuTPMEmulatorUpdateProfileName(virDomainTPMEmulatorDef *emulator,
virCommandAddArgList(cmd, "socket", "--print-info", "0x20", "--tpm2", NULL);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, emulator, persistentTPMDef, cfg);
if (qemuTPMVirCommandSwtpmAddEncryption(cmd, emulator, swtpm) < 0) return -1; @@ -848,7 +852,7 @@ qemuTPMEmulatorBuildCommand(virDomainTPMDef *tpm, virCommandAddArgFormat(cmd, "type=unixio,path=%s,mode=0600", tpm->data.emulator.source->data.nix.path);
- qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator); + qemuTPMVirCommandSwtpmAddTPMState(cmd, &tpm->data.emulator, persistentTPMDef, cfg);
Coverity complains here that the `persistentTPMDef` can be NULL and that it is dereferenced in `qemuTPMHasSharedStorage`.
This is called from `qemuExtDevicesStart` where the for loop calling `qemuExtTPMStart` can actually pass NULL.
Not sure if it can happen with any real VM.
It can, yes. I have a fix ready. Thanks for catching that.
Pavel
virCommandAddArg(cmd, "--log"); if (tpm->data.emulator.debug != 0) -- 2.50.1
On Thu, Jul 17, 2025 at 12:34:40 +0200, Martin Kletzander via Devel wrote:
https://issues.redhat.com/browse/RHEL-80155
Martin Kletzander (3): qemu_tpm: Rename qemuTPMHasSharedStorage -> qemuTPMDomainHasSharedStorage qemu_tpm: Extract per-TPM functionality from qemuTPMDomainHasSharedStorage qemu_tpm: Only warn about missing locking feature on shared filesystems
src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++---------------- src/qemu/qemu_tpm.h | 4 +-- 3 files changed, 47 insertions(+), 34 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On a Thursday in 2025, Martin Kletzander via Devel wrote:
https://issues.redhat.com/browse/RHEL-80155
Martin Kletzander (3): qemu_tpm: Rename qemuTPMHasSharedStorage -> qemuTPMDomainHasSharedStorage qemu_tpm: Extract per-TPM functionality from qemuTPMDomainHasSharedStorage qemu_tpm: Only warn about missing locking feature on shared filesystems
src/qemu/qemu_migration.c | 2 +- src/qemu/qemu_tpm.c | 75 +++++++++++++++++++++++---------------- src/qemu/qemu_tpm.h | 4 +-- 3 files changed, 47 insertions(+), 34 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (4)
-
Ján Tomko -
Martin Kletzander -
Pavel Hrdina -
Peter Krempa