[pve-devel] [PATCH qemu-server] fix #2367: disallow 'PENDING' as snapshot name

[Thomas Lamprecht]

On 11.09.19 12:14, Dominik Csapak wrote:
> this conflicts with our syntax of pending changes, so we have to forbid it
> the check must be case-insensitive since we parse it that way from the config
> 
> this may be reverted again sometime with 7.0 if we decide to change the
> name of the pending section

That cannot really happen, or? I mean I can have now a VM with pending changes
and live migrate it just fine to a next PVE version, still having the changes
pending?

If, one would need to rewrite pending to something else, which cannot possibly
be a snapshot name, 100% safe that could be done at a 7.0 transition (e.h., vm
start incoming) and then allow it again in 8.0, when we can be safe that no such
old pending section can exist?

But IMO: 1. PITA, 2. it'd be easier to keep this dissalowed and add a new
optional snapshot prefix for snapshot sections, which allows it, looking like:
[snap:<snap-name>]

or the-like..

> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  PVE/API2/Qemu.pm  | 3 +++
>  PVE/QemuServer.pm | 2 +-
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 9db8967..9a73d04 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -3683,6 +3683,9 @@ __PACKAGE__->register_method({
>  	die "unable to use snapshot name 'current' (reserved name)\n"
>  	    if $snapname eq 'current';
>  
> +	die "unable to use snapshot name '$snapname' (reserved name)\n"
> +	    if $snapname =~ m/^pending$/i;

FYI, we sometimes use lc for such checks, e.g.:

    if lc($snapname) eq 'pending';

not sure what's faster, personally I prefer lc over regex+/i but that's
probably just pure personal-taste.

Anyway look OK from a quick look. But could we not do that in AbstractConfig
somewhere, to have it for container too (which will have pending sections soon)

> +
>  	my $realcmd = sub {
>  	    PVE::Cluster::log_msg('info', $authuser, "snapshot VM $vmid: $snapname");
>  	    PVE::QemuConfig->snapshot_create($vmid, $snapname, $param->{vmstate},
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 7128723..5295c22 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -2834,7 +2834,7 @@ sub write_vm_config {
>      &$cleanup_config($conf->{pending}, 1);
>  
>      foreach my $snapname (keys %{$conf->{snapshots}}) {
> -	die "internal error" if $snapname eq 'pending';
> +	die "internal error" if $snapname =~ m/^pending$/i;
>  	&$cleanup_config($conf->{snapshots}->{$snapname}, undef, $snapname);
>      }
>  
>