Re: [pve-devel] [PATCH v2 storage 1/9] storage: add new find_free_volname

[Fabian Ebner <f.ebner@proxmox.com>]

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 "<VMID>/" 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 <a.lauterer@proxmox.com>
> ---
> 
> 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) = @_;
>   
>