Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>,
	"Proxmox VE development discussion" <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-storage 04/10] rename_volume: add source && target snap
Date: Fri, 4 Jul 2025 14:04:16 +0200	[thread overview]
Message-ID: <a24d6ce2-4715-4e59-916d-3d78c7fac0b4@proxmox.com> (raw)
In-Reply-To: <1812738791.2234.1751629948403@webmail.proxmox.com>

Am 04.07.25 um 13:52 schrieb Fabian Grünbichler:
>> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 04.07.2025 08:45 CEST geschrieben:
>> allow to rename from|to external snapshot volname
> 
> we could consider adding a new API method `rename_snapshot` instead:
> 
> my ($class, $scfg, $storeid, $volname, $source_snap, $target_snap) = @_;
> 
> for the two plugins here it could easily share most of the implementation
> with rename_volume, without blowing up the interface for a fairly limited
> use case?
> 
> rename_volume right now is used for re-assigning a volume from one
> owner/vmid to another only, AFAICT, with $target_volname never being
> actually provided by callers. the new call would then never provide
> $target_vmid and never provide $target_volname, while existing ones
> never provide the snapshot parameters. OTOH, just like the existing
> rename_volume, such a rename_snapshot method would only have a
> single use case/call site, unless we plan to also add generic
> snapshot renaming as a feature down the line..

I'm currently not too closely into this code, but IMO it might be
indeed a bit nicer to have this as separate and specialized method.
The amount of call sites is not so important to me, having a more
clear and less "multiplexed" API can provide its benefits on it's
own, like avoiding some dangerous edge cases you pointed out below.

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel