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. _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel