[pve-devel] [PATCH qemu-server 1/2] Fix #1924: add snapshot parameter

[Wolfgang Bumiller]

Code is fine, but I'd like some style changes, see inline comments...

On Mon, Nov 12, 2018 at 02:59:58PM +0100, Rhonda D'Vine wrote:
> The config and showcmd CLI commands offer the config and showcmd
> functions. Both of that outputs may vary with respect to a given
> snapshot. This adds a switch that shows the corresponding snapshot's
> config and command line.
> 
> This patch needs the one in pve-guest-common applied first.
> 
> Signed-off-by: Rhonda D'Vine <rhonda at proxmox.com>
> ---
>  PVE/API2/Qemu.pm  | 16 ++++++++++++++++
>  PVE/CLI/qm.pm     | 11 +++++++++--
>  PVE/QemuServer.pm | 12 +++++++++++-
>  debian/control    |  2 +-
>  4 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 52f4a5f..eb514b9 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -828,6 +828,13 @@ __PACKAGE__->register_method({
>  		default => 0,
>  		type => 'boolean',
>              },
> +            snapshot => get_standard_option('pve-snapshot-name', {
> +		description => "Fetch config values from given snapshot.",
> +		optional => 1,
> +		completion => sub {
> +		    PVE::QemuConfig->complete_snapshot_name(@_)
> +		},
> +            }),
>  	},
>      },
>      returns => {
> @@ -845,6 +852,15 @@ __PACKAGE__->register_method({
>  
>  	my $conf = PVE::QemuConfig->load_config($param->{vmid});
>  
> +	if ($param->{snapshot}) {
> +	    die "snapshot '$param->{snapshot}' does not exist\n"
> +		if !defined( $conf->{snapshots}->{$param->{snapshot}});

I'd really like to reduce the number of nested hash accesses and
especially duplicate hash accesses in new code.

Please put

    my $snapshot_name = $param->{snapshot};

before the `if()` (as you're using it as a condition, as name in the
`die()` message, and again as hash key multiple times). And inside the
`if()` start with:

    my $snapshot = $conf->{snapshots}->{$snapshot_name};

Then replace the copies of `$conf->{snapshots}->{$param->{snapshot}}`
with $snapshot.

> +
> +	    # we need the digest of the file
> +	    $conf->{snapshots}->{$param->{snapshot}}->{digest} = $conf->{digest};
> +	    $conf = $conf->{snapshots}->{$param->{snapshot}};

As it would be much more readable this way:

    $snapshot->{digest} = $conf->{digest};
    $conf = $snapshot;

> +	}
> +
>  	delete $conf->{snapshots};
>  
>  	if (!$param->{current}) {
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 46a7e2f..9326dbd 100755
> --- a/PVE/CLI/qm.pm
> +++ b/PVE/CLI/qm.pm
> @@ -126,7 +126,14 @@ __PACKAGE__->register_method ({
>  		type => 'boolean',
>  		optional => 1,
>  		default => 0,
> -	    }
> +	    },
> +	    snapshot => get_standard_option('pve-snapshot-name', {
> +		description => "Fetch config values from given snapshot.",
> +		optional => 1,
> +		completion => sub {
> +		    PVE::QemuConfig->complete_snapshot_name(@_)
> +		}
> +	    }),
>  	},
>      },
>      returns => { type => 'null'},
> @@ -134,7 +141,7 @@ __PACKAGE__->register_method ({
>  	my ($param) = @_;
>  
>  	my $storecfg = PVE::Storage::config();
> -	my $cmdline = PVE::QemuServer::vm_commandline($storecfg, $param->{vmid});
> +	my $cmdline = PVE::QemuServer::vm_commandline($storecfg, $param->{vmid}, $param->{snapshot});
>  
>  	$cmdline =~ s/ -/ \\\n  -/g if $param->{pretty};
>  
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 2bea729..f9a12d1 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -5222,10 +5222,20 @@ sub vm_human_monitor_command {
>  }
>  
>  sub vm_commandline {
> -    my ($storecfg, $vmid) = @_;
> +    my ($storecfg, $vmid, $snapshot) = @_;
>  
>      my $conf = PVE::QemuConfig->load_config($vmid);
>  
> +    if ($snapshot) {

Same here - (it'll easier if the parameter is renamed to
$snapshot_name).

> +	die "snapshot '$snapshot' does not exist\n"
> +	    if !defined( $conf->{snapshots}->{$snapshot});
> +	my $digest = $conf->{digest};
> +
> +	# we need the digest of the file
> +	$conf->{snapshots}->{$snapshot}->{digest} = $conf->{digest};

^ You're not using $digest here.

> +	$conf = $conf->{snapshots}->{$snapshot};
> +    }
> +
>      my $defaults = load_defaults();
>  
>      my $cmd = config_to_command($storecfg, $vmid, $conf, $defaults);
> diff --git a/debian/control b/debian/control
> index f3b9ca0..45d7855 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -5,7 +5,7 @@ Maintainer: Proxmox Support Team <support at proxmox.com>
>  Build-Depends: debhelper (>= 7.0.50~),
>                 libio-multiplex-perl,
>                 libpve-common-perl,
> -               libpve-guest-common-perl (>= 2.0-18),
> +               libpve-guest-common-perl (>> 2.0-18),
>                 lintian,
>                 perl (>= 5.10.0-19),
>                 pve-doc-generator,
> -- 
> 2.11.0