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