[pve-devel] [PATCH qemu-server 1/2] snapshot: fix tpmstate with rbd

[pve-devel] [PATCH qemu-server 1/2] snapshot: fix tpmstate with rbd

[Stefan Reiter]

QEMU doesn't know about the tpmstate, so 'do_snapshots_with_qemu' should
never return true in that case. Note that inconsistencies related to
snapshot timing do not matter much, as the actual TPM data is exported
together with other device state by QEMU anyway.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

As reported in the forum: https://forum.proxmox.com/threads/vtpm-support-do-we-have-guide-to-add-the-vtpm-support.56982/post-423381

 PVE/QemuServer.pm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 6fc28e8..d5dfdbb 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4542,7 +4542,7 @@ sub qemu_volume_snapshot {
 
     my $running = check_running($vmid);
 
-    if ($running && do_snapshots_with_qemu($storecfg, $volid)){
+    if ($running && do_snapshots_with_qemu($storecfg, $volid, $deviceid)) {
 	mon_cmd($vmid, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
     } else {
 	PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
@@ -4564,7 +4564,7 @@ sub qemu_volume_snapshot_delete {
 	});
     }
 
-    if ($running && do_snapshots_with_qemu($storecfg, $volid)){
+    if ($running && do_snapshots_with_qemu($storecfg, $volid, $deviceid)) {
 	mon_cmd($vmid, 'blockdev-snapshot-delete-internal-sync', device => $deviceid, name => $snap);
     } else {
 	PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snap, $running);
@@ -7017,7 +7017,9 @@ my $qemu_snap_storage = {
     rbd => 1,
 };
 sub do_snapshots_with_qemu {
-    my ($storecfg, $volid) = @_;
+    my ($storecfg, $volid, $deviceid) = @_;
+
+    return if $deviceid =~ m/tpmstate0/;
 
     my $storage_name = PVE::Storage::parse_volume_id($volid);
     my $scfg = $storecfg->{ids}->{$storage_name};
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] swtpm: wait for pidfile

[Stefan Reiter]

swtpm may take a little bit to daemonize, so the pidfile might not be
available right after run_command. Causes an ugly warning about using an
undefined value in a match, so wait up to 5s for it to appear.

Note that in testing this loop only ever got to the first or second
iteration, so I believe the timeout duration should be more than enough.

Also add a missing 'usleep' import, 'usleep' was used before but never
imported, apparently the other case never got triggered...

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

Triggered during rollback testing, potentially due to extra load. Didn't cause
any failure (as long as the VM starts fine it's all good), but looks ugly in the
log and may cause lingering instances if QEMU fails afterward.

 PVE/QemuServer.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index d5dfdbb..7a7cdb0 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -22,7 +22,7 @@ use JSON;
 use MIME::Base64;
 use POSIX;
 use Storable qw(dclone);
-use Time::HiRes qw(gettimeofday);
+use Time::HiRes qw(gettimeofday usleep);
 use URI::Escape;
 use UUID;
 
@@ -3059,6 +3059,14 @@ sub start_swtpm {
     push @$emulator_cmd, "--tpm2" if $tpm->{version} eq 'v2.0';
     run_command($emulator_cmd, outfunc => sub { print $1; });
 
+    # swtpm may take a bit to start before daemonizing, wait up to 5s for pid
+    my $tries = 100;
+    while (! -e $paths->{pid}) {
+	usleep(50000);
+	die "failed to start swtpm: pid file '$paths->{pid}' wasn't created.\n"
+	    if --$tries == 0;
+    }
+
     # return untainted PID of swtpm daemon so it can be killed on error
     file_read_firstline($paths->{pid}) =~ m/(\d+)/;
     return $1;
-- 
2.30.2

Re: [pve-devel] [PATCH qemu-server 2/2] swtpm: wait for pidfile

[Thomas Lamprecht]

On 14.10.21 11:28, Stefan Reiter wrote:
> swtpm may take a little bit to daemonize, so the pidfile might not be
> available right after run_command. Causes an ugly warning about using an
> undefined value in a match, so wait up to 5s for it to appear.
> 
> Note that in testing this loop only ever got to the first or second
> iteration, so I believe the timeout duration should be more than enough.
> 
> Also add a missing 'usleep' import, 'usleep' was used before but never
> imported, apparently the other case never got triggered...
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> Triggered during rollback testing, potentially due to extra load. Didn't cause
> any failure (as long as the VM starts fine it's all good), but looks ugly in the
> log and may cause lingering instances if QEMU fails afterward.
> 
>  PVE/QemuServer.pm | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d5dfdbb..7a7cdb0 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -22,7 +22,7 @@ use JSON;
>  use MIME::Base64;
>  use POSIX;
>  use Storable qw(dclone);
> -use Time::HiRes qw(gettimeofday);
> +use Time::HiRes qw(gettimeofday usleep);
>  use URI::Escape;
>  use UUID;
>  
> @@ -3059,6 +3059,14 @@ sub start_swtpm {
>      push @$emulator_cmd, "--tpm2" if $tpm->{version} eq 'v2.0';
>      run_command($emulator_cmd, outfunc => sub { print $1; });
>  
> +    # swtpm may take a bit to start before daemonizing, wait up to 5s for pid
> +    my $tries = 100;
> +    while (! -e $paths->{pid}) {
> +	usleep(50000);
> +	die "failed to start swtpm: pid file '$paths->{pid}' wasn't created.\n"
> +	    if --$tries == 0;

I moved the die before the sleep, as else we'd wait extra 50 ms if we'd die anyway.

> +    }
> +
>      # return untainted PID of swtpm daemon so it can be killed on error
>      file_read_firstline($paths->{pid}) =~ m/(\d+)/;
>      return $1;
> 

[pve-devel] applied: [PATCH qemu-server 1/2] snapshot: fix tpmstate with rbd

[Thomas Lamprecht]

On 14.10.21 11:28, Stefan Reiter wrote:
> QEMU doesn't know about the tpmstate, so 'do_snapshots_with_qemu' should
> never return true in that case. Note that inconsistencies related to
> snapshot timing do not matter much, as the actual TPM data is exported
> together with other device state by QEMU anyway.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> As reported in the forum: https://forum.proxmox.com/threads/vtpm-support-do-we-have-guide-to-add-the-vtpm-support.56982/post-423381
> 
>  PVE/QemuServer.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks! But, ...

> return if $deviceid =~ m/tpmstate0/;

... isn't there a better word boundary we could check for in the match to avoid
(rare but as you used a match vs. equal probably not impossible) false positives?

[pve-devel] applied: [PATCH qemu-server 1/2] snapshot: fix tpmstate with rbd

[Thomas Lamprecht]

On 14.10.21 11:28, Stefan Reiter wrote:
> QEMU doesn't know about the tpmstate, so 'do_snapshots_with_qemu' should
> never return true in that case. Note that inconsistencies related to
> snapshot timing do not matter much, as the actual TPM data is exported
> together with other device state by QEMU anyway.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> As reported in the forum: https://forum.proxmox.com/threads/vtpm-support-do-we-have-guide-to-add-the-vtpm-support.56982/post-423381
> 
>  PVE/QemuServer.pm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
>

applied, thanks!