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.csa...@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);
}
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
