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

[Dominik Csapak]

On 9/11/19 1:26 PM, Thomas Lamprecht wrote:
> 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..

yeah it was just a suggestion which would have been better in the
comment instead of the commit message

i just meant we could allow it again after dealing with the
pending/snapname syntax issue

> 
>>
>> 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.

i have no strong feeling for one or the other but according to
Benchmarks 'cmpthese', a comparison with lc($foo) == 'foo' vs $foo =~ 
m/^foo$/i shows
that lc is about 2 to 3 times faster

> 
> 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)

i am not sure where exactly, i am not so versed in that code, but maybe 
oguz can do that after his pending changes patches are applied?
(or maybe as part of it? @oguz)

> 
>> +
>>   	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);
>>       }
>>   
>>
>