Re: [pve-devel] [PATCH storage] btrfs: check for btrfs in on_add_hook and activate

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

On 24.06.21 11:23, Fabian Grünbichler wrote:
> On June 24, 2021 11:10 am, Thomas Lamprecht wrote:
>> On 24.06.21 09:56, Fabian Grünbichler wrote:
>>> On June 24, 2021 9:29 am, Wolfgang Bumiller wrote:
>>>>  sub activate_storage {
>>>>      my ($class, $storeid, $scfg, $cache) = @_;
>>>> +    assert_btrfs($scfg->{path});
>>>>      return PVE::Storage::DirPlugin::activate_storage($class, $storeid, $scfg, $cache);
>>> shouldn't this be the other way round? first check for things like 
>>> is_mountpoint, then whether btrfs is there.. makes for less confusing 
>>> error message at least..
>>>
>>
>> But then we create already the sub-directories in DirPlugin's SUPER->activate_storage call
>> to the base plugin one and leave that stuff over when the assert fails?
>>
> 
> true. but OTOH, we do support dir storages where $path does not exist 
> yet before the first activation..
> 
> maybe
> 
> if is_mountpoint check that mountpoint // path with DirPlugin::path_is_mounted && btrfs
> 
> then call activate_storage from dir plugin
> 
> then check $path is btrfs
> 
> most setups should have is_mountpoint set (except maybe / on btrfs with 
> no separate "data" filesystem..), so this should handle most of it. if 
> we pull in the mkdir $path handling into the BTRFSPlugin, then 
> everything would be handled (and only the subdir creation is delegate to 
> the DirPlugin..)
> 

I just duplicated the DirPlugin activate storage, could be factored out maybe but
for now I prefer it as is, using actual plugin methods from other plugins feels
always a bit weird and risky, as on the use-site one is seldom aware of that when
changing things there, risking breakage - so in the longer term I'd like that the
more generic stuff moves to a "static" helper module, not having a export-base.