Re: [pve-devel] [RFC storage 1/7] storage: expose find_free_diskname

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

Am 02.07.21 um 15:38 schrieb Aaron Lauterer:
> 
> 
> On 6/2/21 10:29 AM, Fabian Ebner wrote:
>> Am 01.06.21 um 18:10 schrieb Aaron Lauterer:
>>> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
>>> ---
>>>   PVE/Storage.pm | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
>>> index aa36bad..93d09ce 100755
>>> --- a/PVE/Storage.pm
>>> +++ b/PVE/Storage.pm
>>> @@ -201,6 +201,14 @@ sub storage_can_replicate {
>>>       return $plugin->storage_can_replicate($scfg, $storeid, $format);
>>>   }
>>> +sub find_free_diskname {
>>> +    my ($cfg, $storeid, $vmid, $fmt, $add_fmt_suffix) = @_;
>>
>> Nit: Ideally, the $add_fmt_suffix should be decided by the plugin, as 
>> an outside caller cannot know what a plugin wants/expects. Don't know 
>> if that's easy to do the way things are though.
> 
> Yeah, I am not sure how to handle that... most of our storage plugins 
> don't use it at all (ZFS, RBD, LVM). That leaves the Plugin.pm (storage 
> based) and potential 3rd party plugins that have their own implementation.
> Depending on where you would use the now public `find_free_diskname`, it 
> could be possible to know the format and if the suffix should already be 
> added. For the move-disk to other guest (disk reassign) stuff I cannot 
> do that and leave it undefined.
> 

The problem is that half of the plugins will quietly ignore the 
parameter and the other half (dir based plugins) will return a disk name 
they cannot even handle when add_fmt_suffix=0. I'd rather avoid such a 
surprising interface.

The same criticism applies to the plugin's interface, but at least there 
the calls come from the plugins themselves, which know what they need/want.

Two potential solutions:

1) Remove the add_fmt_suffix parameter from the plugin's 
find_free_diskname and either:
a) add custom implementations to the non-dir-based plugins, never adding 
the suffix.
b) adapt the generic implementation to add the suffix depending on 
whether $scfg->{path} is present or not.

2) Add a new function to the plugin interface, returning whether the 
plugin expects a suffix to be added or not, and use that in this patch's 
proposed find_free_diskname.

IMHO 1) is much cleaner, but also a breaking change to the plugin API. 
But currently APIAGE is 0, so it wouldn't be as bad.

> In the Plugin.pm implementation of `rename_volume` I added a check if a 
> suffix is present, and if not, take the one from the source volume.
> Thinking about it, I might even strip any potential suffixes and always 
> use the one from the source as that should be kept since the file format 
> probably should not change with a rename.
> 
>>
>>> +
>>> +    my $scfg = storage_config($cfg, $storeid);
>>> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
>>> +    return $plugin->find_free_diskname($storeid, $scfg, $vmid, $fmt, 
>>> $add_fmt_suffix);
>>> +}
>>> +
>>>   sub storage_ids {
>>>       my ($cfg) = @_;
>>>