Re: [pve-devel] [PATCH v2 guest-common 2/2] snapshots: abort if new snapshot name is already parent to existing one

[Fabian Ebner <f.ebner@proxmox.com>]

Am 13.10.21 um 14:31 schrieb Oguz Bektas:
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>   src/PVE/AbstractConfig.pm | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
> index 3348d8a..6849664 100644
> --- a/src/PVE/AbstractConfig.pm
> +++ b/src/PVE/AbstractConfig.pm
> @@ -721,14 +721,22 @@ sub __snapshot_prepare {
>   
>   	$conf->{lock} = 'snapshot';
>   
> +	my $snapshots = $conf->{snapshots};
> +
>   	die "snapshot name '$snapname' already used\n"
> -	    if defined($conf->{snapshots}->{$snapname});
> +	    if defined($snapshots->{$snapname});
>   
>   	my $storecfg = PVE::Storage::config();
>   	die "snapshot feature is not available\n"
>   	    if !$class->has_feature('snapshot', $conf, $storecfg, undef, undef, $snapname eq 'vzdump');
>   
> -	$snap = $conf->{snapshots}->{$snapname} = {};
> +	foreach my $existing_snapshot (keys %$snapshots) {
> +	    my $parent_name = $snapshots->{$existing_snapshot}->{parent} // '';
> +	    die "snapshot '$snapname' cannot be used, $snapname already a parent for $existing_snapshot\n"
> +		if $snapname eq $parent_name;
> +	}

Personally, I'd prefer the "automatically fix it up"-approach, because 
we we're the ones introducing the invalid property in the first place.

But if we go for the "error out"-approach, the error message should be 
different. You're telling the user that the snapshot name cannot be 
used, but they'll just wonder why. Instead, the error should rather tell 
the user that manual fix-up is required and that the parent property is 
invalid, because the parent doesn't exist.

> +
> +	$snap = $snapshots->{$snapname} = {};
>   
>   	if ($save_vmstate && $class->__snapshot_check_running($vmid)) {
>   	    $class->__snapshot_save_vmstate($vmid, $conf, $snapname, $storecfg);
>