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

[Mira Limbeck]

I'll rework the patch then.

On 4/26/19 5:06 PM, Thomas Lamprecht wrote:
> 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) = ...