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 D00B16D39D for ; Fri, 13 Aug 2021 14:47:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C07472C7FB for ; Fri, 13 Aug 2021 14:47:08 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 1C45A2C7EE for ; Fri, 13 Aug 2021 14:47:04 +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 E483343315 for ; Fri, 13 Aug 2021 14:46:57 +0200 (CEST) To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20210806134647.632054-1-a.lauterer@proxmox.com> <20210806134647.632054-2-a.lauterer@proxmox.com> From: Aaron Lauterer Message-ID: <2619d761-5035-9b07-d6d1-fbb3e5fe6265@proxmox.com> Date: Fri, 13 Aug 2021 14:46:51 +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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.307 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [storage.pm, plugin.pm] 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: Fri, 13 Aug 2021 12:47:08 -0000 On 8/12/21 2:50 PM, Fabian Ebner wrote: > 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. Okay I see your point. So following the regular approach throughout the pve-storage repo should be okay. That is, evaluating $scfg->{path} directly. > >> +    return "${vmid}/$diskname"; >> +    } >> +    return $diskname; >> +} >> + >>   sub clone_image { >>       my ($class, $scfg, $storeid, $volname, $vmid, $snap) = @_; >>