Re: [pve-devel] [PATCH v3 storage 3/4] add disk reassign feature

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 18.09.20 17:07, Aaron Lauterer wrote:
> 
> 
> On 9/18/20 4:24 PM, Thomas Lamprecht wrote:
>> On 9/10/20 4:32 PM, Aaron Lauterer wrote:
>>> Functionality has been added for the following storage types:
>>>
>>> * dir based ones
>>>      * directory
>>>      * NFS
>>>      * CIFS
>>>      * gluster
>>> * ZFS
>>> * (thin) LVM
>>> * Ceph
>>>
>>> A new feature `reassign` has been introduced to mark which storage
>>> plugin supports the feature.
>>>
>>> A new intermediate class for directory based storages has been
>>> introduced. This was necessary to maintain compatibility with third
>>> party storage plugins and to avoid duplicate code in the dir based
>>> plugins.
>>>
>>> The new `BaseDirPlugin.pm` adds the `reassign` feature flag and
>>> containes the implementation for the reassign functionlity.
>>>
>>> In the future all the directory specific code in Plugin.pm should be
>>> moved to the BaseDirPlugin.pm. But this will most likely break
>>> compatibility with third party plugins and should thus be done with
>>> care.
>>
>> how so? why don't you just add it in plugin.pm with either:
>> * a general directory based implementation, and a no-op/die for the other
>>    ones
> 
> That was the first approach, but this would mean a die for all other plugins and if 3rd party plugins would not implement it, the directory based code will be used. But we cannot know if if the directory approach is the right one.

Note that you also can check the APIVERSION of external plugins through
the "api" method, which is required for them and checked on loading.

> 
>> * a dummy "die implement me in subclass" method if above is not possible
> 
> If I do it this way, I cannot put the actual code for dir based storage in the Plugin.pm, thus having an intermediate class for dir based storages would still be beneficial IMHO to avoid code duplication. IIRC we have 4 dir based storages (dir, CIFS, NFS, Gluster).

code duplication can also be avoided by just having a general method somewhere
which the single plugins just wrap around (call). That just "duplicates"
the calls, which is no actual duplication as it provides concrete information
about what which plugin uses they all share the implementation, which is the
actual reason for avoiding code duplication.

IMO a saner and more understandable than this approach, plus external plugins
can just call into this if it works for them.