[pve-devel] [PATCH storage] map_volume: fallback to 'path'

[Thomas Lamprecht]

Am 4/26/19 um 4:36 PM schrieb Mira Limbeck:
> 
> On 4/26/19 4:30 PM, Thomas Lamprecht wrote:
>> Am 4/26/19 um 3:00 PM schrieb Mira Limbeck:
>>> Adds a fallback to 'Plugin::path' in the default implementation of
>>> 'map_volume' to make the additional call to 'path' unnecessary if
>>> 'map_volume' is not implemented in the plugin used. 'Plugin::path' is now
>>> always returned if the plugin in question does not override 'map_volume'.
>>>
>>> Signed-off-by: Mira Limbeck <m.limbeck at proxmox.com>
>>> ---
>>> This change was discussed with @Wolfgang off-list. It should simplify
>>> the use-case of calling 'map_volume' followed by 'path' if it is not
>>> defined as found for example in pve-container/src/API2/LXC.pm:
>>> my $path = PVE::Storage::map_volume($storage_cfg, $volid);
>>> $path = PVE::Storage::path($storage_cfg, $volid) if !defined($path);
>>>
>>>   PVE/Storage/Plugin.pm | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
>>> index 7964441..6b428d7 100644
>>> --- a/PVE/Storage/Plugin.pm
>>> +++ b/PVE/Storage/Plugin.pm
>>> @@ -966,7 +966,8 @@ sub deactivate_storage {
>>>   sub map_volume {
>>>       my ($class, $storeid, $scfg, $volname, $snapname) = @_;
>>>   -    return undef;
>>> +    my ($path, $vmid, $vtype) = $class->path($scfg, $volname, $storeid, $snapname);
>>> +    return wantarray ? ($path, $vmid, $vtype) : $path;
>> why wantarray and not just returning the $path, like our single actual
>> implementation of map_volume in RBD does? What's the rationale?
> To match the path call in PVE/Storage.pm, returning just the path should be enough, yes.

but we do not want to copy the whole path behavior, as then we
could have just used that (and possible added mapping there),
just $path is enough, all else feels weird and as if we want to
make all methods multipurpose ones.

>>
>> And if there'd be one it still makes sense to have the return
>> signatures in sync... But as it does not fits the map, which has no
>> interest in $vmid or $vtype, but only at which path the volume is
>> accessible (i.e., mapped) I doubt there is one.
>>
>> also, why not just:
>>
>> return $class->path($scfg, $volname, $storeid, $snapname);
> This returned $vtype in my tests, not $path on ZFS. Works on lvm-thin and directory storages. The implementation in ZFSPoolPlugin.pm always returns an array containing all 3.

Ah yeah, the single thing plugin path method implementations _must_ follow is
returning ($path, $vmid, $vtype) in list context, as the PVE::Storage::path
method then does the wantarray logic, so just a: my ($path) = ...