On Tue, Jun 12, 2018 at 02:48:48PM +0200, Thomas Lamprecht wrote: > On 6/11/18 11:31 AM, Alwin Antreich wrote: > > - ability to mount through kernel and fuse client > > - allow mount options > > - get MONs from ceph config if not in storage.cfg > > - allow the use of ceph config with fuse client > > (...) > > +sub cephfs_is_mounted { > > + my ($scfg, $storeid, $mountdata) = @_; > > + > > + my $cmd_option = PVE::CephStorageTools::ceph_connect_option($scfg, > > $storeid); > > + my $configfile = $cmd_option->{ceph_conf}; > > + my $server = $get_monaddr_list->($scfg, $configfile); > > + > > + my $subdir = $scfg->{subdir} ? $scfg->{subdir} : '/'; > > We normally use the following style for this: > > my $subdir = $scfg->{subdir} // '/';
Note the subtle difference - // checks for defined()ness of $scfg->{subdir} in this case which you were probably lacking, depending on whether explicit empty paths are allowed... > > + my $mountpoint = $scfg->{path}; > > + my $source = "$server:$subdir"; > > + > > + $mountdata = PVE::ProcFSTools::parse_proc_mounts() if !$mountdata; > > + return $mountpoint if grep { > > + $_->[2] =~ m#^ceph|fuse\.ceph-fuse# && > > + $_->[0] =~ m#^\Q$source\E|ceph-fuse$# && > > + $_->[1] eq $mountpoint > > + } @$mountdata; > > + > > + warn "A filesystem is already mounted on $mountpoint\n" > > + if grep { $_->[1] eq $mountpoint } @$mountdata; > > + > > + return undef; > > +} > > + (...) > > + > > +sub activate_storage { > > + my ($class, $storeid, $scfg, $cache) = @_; > > + > > + $cache->{mountdata} = PVE::ProcFSTools::parse_proc_mounts() > > + if !$cache->{mountdata}; > > + > > + my $path = $scfg->{path}; > > + > > + if (!cephfs_is_mounted($scfg, $storeid, $cache->{mountdata})) { > > + > > + # NOTE: only call mkpath when not mounted (avoid hang > > + # when cephfs is offline > > + > > + mkpath $path if !(defined($scfg->{mkdir}) && !$scfg->{mkdir}); > > what? I do not understand this, who set's mkdir and why should it > tell us if cephfs is not mounted? > I'd just omit above comment, it's more confusing than helpful, IMO. It's our directory-based storage option to know whether to create the path leading up to the storage, as doing so when there's an intermediate mount-point missing might prevent the mount from happening. (eg. zfs) But the comment isn't about this condition, it's about the one 2 lines further up, so move it above the if, or remove the empty line in between, to group them together. Btw., since we're mounting a directory, it would probably be nice to also allow adding the 'is_mountpoint' condition (particularly the path based variant is always useful). _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel