Re: [pve-devel] [PATCH qemu-server 2/4] savevm: periodically print progress

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

much thanks in general for following up on my request, this irked
me since almost ever.

a few nits inline.

On 04.02.21 13:52, Stefan Reiter wrote:
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/QemuConfig.pm | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 3f4605f..37db347 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -14,6 +14,7 @@ use PVE::QemuServer;
>  use PVE::QemuServer::Machine;
>  use PVE::Storage;
>  use PVE::Tools;
> +use PVE::CLIFormatter;
>  
>  use base qw(PVE::AbstractConfig);
>  
> @@ -280,14 +281,26 @@ sub __snapshot_create_vol_snapshots_hook {
>  		PVE::Storage::activate_volumes($storecfg, [$snap->{vmstate}]);
>  
>  		mon_cmd($vmid, "savevm-start", statefile => $path);
> +		print "saving VM state and RAM\n";
> +		my $start = time();

The SaveVMInfo struct returned by 'query-savevm' would have a 'total-time' field,
why not use that?

> +		my $state = sub {

over general helper name

> +		    my ($bytes) = @_;
> +		    my $b = PVE::CLIFormatter::render_bytes($bytes);
> +		    my $t = PVE::CLIFormatter::render_duration(time() - $start);
> +		    return "$b in $t";

could return a ($b, $t) tuple here, just an idea, this is totally fine to
with the current usage.

> +		};
>  		for(;;) {
>  		    my $stat = mon_cmd($vmid, "query-savevm");
>  		    if (!$stat->{status}) {
>  			die "savevm not active\n";
>  		    } elsif ($stat->{status} eq 'active') {
>  			sleep(1);
> +			my $s = $state->($stat->{bytes});
> +			print "progress: $s\n";

I'd drop the "progress", IMO just noise.

>  			next;
>  		    } elsif ($stat->{status} eq 'completed') {
> +			my $s = $state->($stat->{bytes});


> +			print "saved $s\n";

I'd explicitly out put that we're done:

"completed, saved $s\n";

>  			last;
>  		    } else {
>  			die "query-savevm returned status '$stat->{status}'\n";
>