From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 4A71A6CFFA for ; Thu, 12 Aug 2021 14:51:35 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 41BB723483 for ; Thu, 12 Aug 2021 14:51:05 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 920BB23474 for ; Thu, 12 Aug 2021 14:51:00 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 6A9B0432F4 for ; Thu, 12 Aug 2021 14:51:00 +0200 (CEST) To: pve-devel@lists.proxmox.com, Aaron Lauterer References: <20210806134647.632054-1-a.lauterer@proxmox.com> <20210806134647.632054-2-a.lauterer@proxmox.com> From: Fabian Ebner Message-ID: Date: Thu, 12 Aug 2021 14:50:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20210806134647.632054-2-a.lauterer@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.405 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v2 storage 1/9] storage: add new find_free_volname X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Aug 2021 12:51:35 -0000 Am 06.08.21 um 15:46 schrieb Aaron Lauterer: > This new method exposes the functionality to request a new, not yet > used, volname for a storage. > > The default implementation will return the result from > 'find_free_diskname' prefixed with "/" if $scfg->{path} exists. > Otherwise it will only return the result from 'find_free_diskname'. > > If the format suffix is added also depends on the existence of > $scfg->{path}. > > $scfg->{path} will be present for directory based storage types. > > Should a storage need to return a different volname, it needs to override > the 'find_free_volname' method. > > Signed-off-by: Aaron Lauterer > --- > > v1 -> v2: > * drop exposed 'find_free_diskname' in favor of 'find_free_volname' > * dropped 'wants_fmt_suffix' as 'find_free_volname' now decides that itself > > rfc -> v1: > dropped $add_fmt_suffix parameter and added the "wants_fmt_suffix" > helper method in each plugin. > > PVE/Storage.pm | 9 +++++++++ > PVE/Storage/Plugin.pm | 11 +++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/PVE/Storage.pm b/PVE/Storage.pm > index c04b5a2..c38fe7b 100755 > --- a/PVE/Storage.pm > +++ b/PVE/Storage.pm > @@ -203,6 +203,15 @@ sub storage_can_replicate { > return $plugin->storage_can_replicate($scfg, $storeid, $format); > } > > +sub find_free_volname { > + my ($cfg, $storeid, $vmid, $fmt) = @_; > + > + my $scfg = storage_config($cfg, $storeid); > + my $plugin = PVE::Storage::Plugin->lookup($scfg->{type}); > + > + return $plugin->find_free_volname($storeid, $scfg, $vmid, $fmt); > +} > + > sub storage_ids { > my ($cfg) = @_; > > giff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm > index b1865cb..e043329 100644 > --- a/PVE/Storage/Plugin.pm > +++ b/PVE/Storage/Plugin.pm > @@ -685,6 +685,17 @@ sub find_free_diskname { > return get_next_vm_diskname($disk_list, $storeid, $vmid, $fmt, $scfg, $add_fmt_suffix); > } > > +sub find_free_volname { > + my ($class, $storeid, $scfg, $vmid, $fmt) = @_; > + > + my $diskname = $class->find_free_diskname($storeid, $scfg, $vmid, $fmt, exists($scfg->{path})); > + > + if (exists($scfg->{path})) { More of a nit, but I don't like the usage of exists here, for two reasons: 1. The main reason: it's inconsistent with all the other checks for $scfg->{path} that are used throughout the file. 2. It's more likely to break. Consider some code I've made up, but similar code might exist somewhere in our code base: ... my $path = get_path_from_config($cfg, $storeid); my $scfg = { path => $path, content => $content, ... }; or my $start = $scfg->{path}[0]; # auto-vivication means the 'path' key now always exists (with undef value if it was undef before) if (defined($start) && $start eq '/') { ... } Using exists here makes it necessary to check that such code is not used for $scfg, currently and in the future. > + return "${vmid}/$diskname"; > + } > + return $diskname; > +} > + > sub clone_image { > my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_; > >