1. Patchew
  2. Libvirt
  3. [PATCH 0/3] qemu_tpm: Do not pollute logs with unnecessary warning
  4. View patch

[PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems

Martin Kletzander via Devel posted 3 patches 1 month, 3 weeks ago
[PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems
Posted by Martin Kletzander via Devel 1 month, 3 weeks ago 
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
Re: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems
Posted by Pavel Hrdina via Devel 1 month, 3 weeks ago 
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
>
Re: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems
Posted by Martin Kletzander via Devel 1 month, 3 weeks ago 
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
>>
Re: [PATCH 3/3] qemu_tpm: Only warn about missing locking feature on shared filesystems
Posted by Ján Tomko via Devel 1 month, 3 weeks ago 
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 = "";
>     }
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.