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 3490D7D2DD for ; Mon, 8 Nov 2021 16:53:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D5C202889A for ; Mon, 8 Nov 2021 16:53:36 +0100 (CET) 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 A02762888D for ; Mon, 8 Nov 2021 16:53:35 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7313D45E06 for ; Mon, 8 Nov 2021 16:53:35 +0100 (CET) Message-ID: <96a51277-818a-43d6-0849-a311d4b3c9f3@proxmox.com> Date: Mon, 8 Nov 2021 16:53:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101 Thunderbird/95.0 Content-Language: en-US To: Proxmox VE development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20211102150335.2371545-1-a.lauterer@proxmox.com> <20211102150335.2371545-2-a.lauterer@proxmox.com> <1636384211.m34yji3cck.astroid@nora.none> From: Thomas Lamprecht In-Reply-To: <1636384211.m34yji3cck.astroid@nora.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 1.176 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 -2.039 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 v4 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: Mon, 08 Nov 2021 15:53:37 -0000 On 08.11.21 16:21, Fabian Gr=C3=BCnbichler wrote: > On November 2, 2021 4:03 pm, Aaron Lauterer wrote: >> 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 different volname, it needs to overrid= e >> the 'find_free_volname' method. >> >> Signed-off-by: Aaron Lauterer >=20 > as discussed off-list (I am sorry I didn't read through the full=20 > discussion on-list of all 10 versions of this and the previous series=20 > ;)) - IMHO this is not a backwards compatible addition since it's not=20 > guarded by any feature flag, so external plugins fall into two=20 > categories: > - derived from Plugin.pm, so automatically implement it via=20 > find_free_diskname -> get_next_vm_diskname, which might or might not = > be correct for that plugin > - not derived from Plugin.pm, don't implement it, no safeguard to handl= e=20 > that >=20 > RIGHT NOW, the only callers are guarded by the rename feature so there = > can't be any problems in practice - but there is no guarantee that this= =20 > will be remembered down the line, and it also breaks the=20 > modularization/layering of the code to rely on this fact ;) >=20 > while we could argue about whether we really want to be that strict,=20 > another point against exposing this method for me is that it's also=20 > prone to mis-use, since it encourages code patterns like in this=20 > series with: >=20 > get_free_volname > do_something_that_allocates_that_volname >=20 > without holding a storage lock over both calls, since the storage lock = > is only available on the plugin level and not exposed in PVE::Storage=20 > itself. for the record, that's not a problem in the sense of consistency or the l= ike, the alloc is the important thing and that is locked, it can be a nuisance= as one can gets their rename rejected, but that's not uncommon in our API in= general. IIRC my idea was that the user (has to) get a free name proposed (i.e., t= he client can request a free name for $owner and submits that, so that they = are in the loop about the new name already, if the API call returns OK then a= ll worked out, else, well not -> check error. That way we avoid that the user has no idea about what the new volid will= be and they won't have to parse some (task) logs or guess around.. >=20 > an interface like > $plugin->rename_volume($scfg, $storeid, $source, $new_owner, $new_name)= >=20 > with at least $new_owner or $new_name set would also cover both cases=20 > (reassign with just $new_owner, rename with just $new_name) and can be = > feature-guarded while keeping the locking in PVE::Storage for the full = > operation including finding a free slot if needed. >=20 > alternatively, just implementing >=20 > $plugin->reassign_volume($scfg, $storeid, $source, $new_owner) >=20 > for now (guarded by a 'reassign' feature), and postponing the rename=20 > feature for a future series that handles 'custom suffix/volname'=20 > across the board of our stack would also be an option. >=20