[pve-devel] [PATCH v1 storage 1/9] storage: expose find_free_diskname

[Fabian Ebner]

Am 19.07.21 um 16:52 schrieb Aaron Lauterer:
> We do not expose the parameter 'add_fmt_suffix' used by the internal
> implemantion of 'find_free_diskname'. This is something only the plugins
> themselves know but cannot be determined easily and reliably from an
> outside caller.
> 
> This is why the new 'wants_fmt_suffix' method has been introduced. For
> most plugins the return value is very clear. For the default
> implementation in Plugin.pm we add another check to be on the safe side
> and only return true if the '$scfg->{path}' option is present.
> It indicates that the volume in question is an actual file which will
> need the suffix.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> --- > rfc -> v1:
> dropped $add_fmt_suffix parameter and added the "wants_fmt_suffix"
> helper method in each plugin.
> 
>   PVE/Storage.pm               | 11 +++++++++++
>   PVE/Storage/LVMPlugin.pm     |  5 +++++
>   PVE/Storage/Plugin.pm        |  7 +++++++
>   PVE/Storage/RBDPlugin.pm     |  5 +++++
>   PVE/Storage/ZFSPoolPlugin.pm |  5 +++++
>   5 files changed, 33 insertions(+)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index c04b5a2..afeb2e3 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -203,6 +203,17 @@ sub storage_can_replicate {
>       return $plugin->storage_can_replicate($scfg, $storeid, $format);
>   }
>   
> +sub find_free_diskname {
> +    my ($cfg, $storeid, $vmid, $fmt) = @_;
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +    my $add_fmt_suffix = $plugin->wants_fmt_suffix($scfg);
> +
> +    return $plugin->find_free_diskname($storeid, $scfg, $vmid, $fmt, $add_fmt_suffix);

Maybe this should rather be find_free_volname and always return the full 
volname (i.e. "<VMID>/vm-<VMID>-disk-<N>.<FMT>" for dir-based storages) 
so that it can be parsed with parse_volname later?

That would mean something like wants_vmid_prefix() is needed, but I'd 
rather extend (and rename) the wants_fmt_suffix method instead.

The rationale is: the value returned here is used for the 
$target_volname parameter in the next patch, and in case it's a 
dir-based storage, passing in an actual volname like 
'123/vm-123-disk-0.raw' currently fails (because it actually expects 
only the disk name).

> +}
> +
>   sub storage_ids {
>       my ($cfg) = @_;
>   
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 139d391..3e5b6c8 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -201,6 +201,11 @@ sub type {
>       return 'lvm';
>   }
>   
> +sub wants_fmt_suffix {
> +    my ($class, $scfg) = @_;
> +    return 0;
> +}
> +

Nit: since there is no $scfg->{path}, the default implementation would 
already return 0. Same for {RBD,ZFSPool}Plugin below, but overwriting 
the method doesn't really hurt either of course.

>   sub plugindata {
>       return {
>   	content => [ {images => 1, rootdir => 1}, { images => 1 }],
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index b1865cb..5c6c659 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -191,6 +191,13 @@ sub default_format {
>       return wantarray ? ($def_format, $valid_formats) : $def_format;
>   }
>   
> +sub wants_fmt_suffix {
> +    my ($class, $scfg) = @_;
> +    return 1 if $scfg->{path};
> +    return 0;
> +}
> +
> +
>   PVE::JSONSchema::register_format('pve-storage-path', \&verify_path);
>   sub verify_path {
>       my ($path, $noerr) = @_;
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index a8d1243..86ea45a 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -273,6 +273,11 @@ sub type {
>       return 'rbd';
>   }
>   
> +sub wants_fmt_suffix {
> +    my ($class, $scfg) = @_;
> +    return 0;
> +}
> +
>   sub plugindata {
>       return {
>   	content => [ {images => 1, rootdir => 1}, { images => 1 }],
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index c4be70f..85e2211 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -18,6 +18,11 @@ sub type {
>       return 'zfspool';
>   }
>   
> +sub wants_fmt_suffix {
> +    my ($class, $scfg) = @_;
> +    return 0;
> +}
> +
>   sub plugindata {
>       return {
>   	content => [ {images => 1, rootdir => 1}, {images => 1 , rootdir => 1}],
>