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: >>> Signed-off-by: Wolfgang Bumiller <w.bumil...@proxmox.com> >>> --- >>> PVE/Storage/BTRFSPlugin.pm | 30 ++++++++++++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/PVE/Storage/BTRFSPlugin.pm b/PVE/Storage/BTRFSPlugin.pm >>> index 133edc6..0e111a0 100644 >>> --- a/PVE/Storage/BTRFSPlugin.pm >>> +++ b/PVE/Storage/BTRFSPlugin.pm >>> @@ -20,6 +20,7 @@ use constant { >>> FS_NOCOW_FL => 0x00800000, >>> FS_IOC_GETFLAGS => 0x40086602, >>> FS_IOC_SETFLAGS => 0x80086601, >>> + BTRFS_MAGIC => 0x9123683e, >>> }; >>> >>> # Configuration (similar to DirPlugin) >>> @@ -89,8 +90,29 @@ sub check_config { >>> return PVE::Storage::DirPlugin::check_config($self, $sectionId, >>> $config, $create, $skipSchemaCheck); >>> } >>> >>> +my sub getfsmagic($) { >>> + my ($path) = @_; >>> + # The field type sizes in `struct statfs` are defined in a rather >>> annoying way, and we only >>> + # need the first field, which is a `long` for our supported platforms. >>> + # Should be moved to pve-rs, so this can be the problem of the `libc` >>> crate ;-) >>> + # Just round up and extract what we need: >>> + my $buf = pack('x160'); >>> + if (0 != syscall(&PVE::Syscall::SYS_statfs, $path, $buf)) { >>> + die "statfs on '$path' failed - $!\n"; >>> + } >>> + >>> + return unpack('L!', $buf); >>> +} >>> + >>> +my sub assert_btrfs($) { >>> + my ($path) = @_; >>> + die "'$path' is not a btrfs file system\n" >>> + if getfsmagic($path) != BTRFS_MAGIC; >>> +} >>> + >>> 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..) _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel