From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <pve-devel-bounces@lists.proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id BBC521FF189 for <inbox@lore.proxmox.com>; Fri, 4 Apr 2025 14:17:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 55EA81DDD5; Fri, 4 Apr 2025 14:17:40 +0200 (CEST) Message-ID: <b63f5ef9-e6e1-40ac-be04-d5e2d3ac76b0@proxmox.com> Date: Fri, 4 Apr 2025 14:17:06 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>, Markus Frank <m.frank@proxmox.com> References: <20250403103442.136958-1-m.frank@proxmox.com> <20250403103442.136958-3-m.frank@proxmox.com> Content-Language: en-US From: Daniel Kral <d.kral@proxmox.com> In-Reply-To: <20250403103442.136958-3-m.frank@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.012 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [tunnel.pm, storagetunnel.pm, dir.pm, pci.pm, usb.pm] Subject: Re: [pve-devel] [PATCH guest-common v15 2/12] add dir mapping section config X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com> Two comments inline. On 4/3/25 12:34, Markus Frank wrote: > Adds a config file for directories by using a 'map' property string for > each node mapping. > > example config: > ``` > some-dir-id > map node=node1,path=/path/to/share/ > map node=node2,path=/different/location/ > ``` > > Signed-off-by: Markus Frank <m.frank@proxmox.com> > --- > v15: > * removed announce-submounts option altogether and always set it for > virtiofsd > * assert_valid_map_list: only run assert_valid for the mappings on the > current node > > src/Makefile | 1 + > src/PVE/Mapping/Dir.pm | 192 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 193 insertions(+) > create mode 100644 src/PVE/Mapping/Dir.pm > > diff --git a/src/Makefile b/src/Makefile > index cbc40c1..030e7f7 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -15,6 +15,7 @@ install: PVE > install -m 0644 PVE/StorageTunnel.pm ${PERL5DIR}/PVE/ > install -m 0644 PVE/Tunnel.pm ${PERL5DIR}/PVE/ > install -d ${PERL5DIR}/PVE/Mapping > + install -m 0644 PVE/Mapping/Dir.pm ${PERL5DIR}/PVE/Mapping/ > install -m 0644 PVE/Mapping/PCI.pm ${PERL5DIR}/PVE/Mapping/ > install -m 0644 PVE/Mapping/USB.pm ${PERL5DIR}/PVE/Mapping/ > install -d ${PERL5DIR}/PVE/VZDump > diff --git a/src/PVE/Mapping/Dir.pm b/src/PVE/Mapping/Dir.pm > new file mode 100644 > index 0000000..4673f83 > --- /dev/null > +++ b/src/PVE/Mapping/Dir.pm > @@ -0,0 +1,192 @@ > +package PVE::Mapping::Dir; > + > +use strict; > +use warnings; > + > +use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_lock_file cfs_write_file); > +use PVE::INotify; > +use PVE::JSONSchema qw(get_standard_option parse_property_string); > +use PVE::SectionConfig; > + > +use base qw(PVE::SectionConfig); > + > +my $FILENAME = 'mapping/dir.cfg'; > + > +cfs_register_file($FILENAME, > + sub { __PACKAGE__->parse_config(@_); }, > + sub { __PACKAGE__->write_config(@_); }); > + > + > +# so we don't have to repeat the type every time > +sub parse_section_header { > + my ($class, $line) = @_; > + > + if ($line =~ m/^(\S+)\s*$/) { > + my $id = $1; > + my $errmsg = undef; # set if you want to skip whole section > + eval { PVE::JSONSchema::pve_verify_configid($id) }; > + $errmsg = $@ if $@; > + my $config = {}; # to return additional attributes > + return ('dir', $id, $errmsg, $config); > + } > + return undef; > +} > + > +sub format_section_header { > + my ($class, $type, $sectionId, $scfg, $done_hash) = @_; > + > + return "$sectionId\n"; > +} > + > +sub type { > + return 'dir'; > +} > + > +# temporary path format that also disallows commas and equal signs > +# TODO: Remove this when property_string supports quotation of properties > +PVE::JSONSchema::register_format('pve-storage-path-in-property-string', \&verify_path); > +sub verify_path { > + my ($path, $noerr) = @_; > + > + if ($path !~ m|^/[^;,=\(\)]+|) { > + return undef if $noerr; > + die "Value does not look like a valid absolute path." > + ." These symbols are currently not allowed in path: ;,=()\n"; > + } > + return $path; > +} nit: usually I've seen these defined as `sub pve_verify_*`, but I'm not sure if we have anything set in stone here. > + > +my $map_fmt = { > + node => get_standard_option('pve-node'), > + path => { > + description => "Absolute directory path that should be shared with the guest.", > + type => 'string', > + format => 'pve-storage-path-in-property-string', > + }, > +}; > + > +my $defaultData = { > + propertyList => { > + id => { > + type => 'string', > + description => "The ID of the directory mapping", > + format => 'pve-configid', > + }, > + description => { > + type => 'string', > + description => "Description of the directory mapping", > + optional => 1, > + maxLength => 4096, > + }, > + map => { > + type => 'array', > + description => 'A list of maps for the cluster nodes.', > + optional => 1, > + items => { > + type => 'string', > + format => $map_fmt, > + }, > + }, > + }, > +}; > + > +sub private { > + return $defaultData; > +} > + > +sub options { > + return { > + description => { optional => 1 }, > + map => {}, > + }; > +} > + > +sub assert_valid { > + my ($dir_cfg) = @_; > + > + my $path = $dir_cfg->{path}; > + > + verify_path($path); > + > + if (! -e $path) { > + die "Path $path does not exist\n"; > + } elsif (! -d $path) { > + die "Path $path exists, but is not a directory\n"; > + } > + > + return 1; > +}; > + > +sub assert_valid_map_list { > + my ($map_list) = @_; > + > + my $nodename = PVE::INotify::nodename(); > + > + my %count; > + for my $map (@$map_list) { > + my $entry = parse_property_string($map_fmt, $map); > + if ($entry->{node} eq $nodename) { > + assert_valid($entry); > + } I see why this wouldn't work otherwise, but it's a shame that when creating the dir mappings for multiple nodes on a single node, whether the path actually exists in the filesystem is only checked for the current node here when adding the dir mapping. It would be a nice to have if we could error out for other nodes here too, but it's not a deal breaker as this correctly asserts for the important parts, that is when starting the VM. > + $count{$entry->{node}}++; > + } > + for my $node (keys %count) { > + if ($count{$node} > 1) { > + die "Node '$node' is specified $count{$node} times.\n"; > + } > + } > +} > + > +sub config { > + return cfs_read_file($FILENAME); > +} > + > +sub lock_dir_config { > + my ($code, $errmsg) = @_; > + > + cfs_lock_file($FILENAME, undef, $code); > + if (my $err = $@) { > + $errmsg ? die "$errmsg: $err" : die $err; > + } > +} > + > +sub write_dir_config { > + my ($cfg) = @_; > + > + cfs_write_file($FILENAME, $cfg); > +} > + > +sub find_on_current_node { > + my ($id) = @_; > + > + my $cfg = config(); > + my $node = PVE::INotify::nodename(); > + > + my $node_mapping = get_node_mapping($cfg, $id, $node); > + if (@{$node_mapping} > 1) { > + die "More than than one directory mapping for node $node.\n"; > + } > + return $node_mapping->[0]; > +} > + > +sub get_node_mapping { > + my ($cfg, $id, $nodename) = @_; > + > + return undef if !defined($cfg->{ids}->{$id}); > + > + my $res = []; > + my $mapping_list = $cfg->{ids}->{$id}->{map}; > + for my $map (@{$mapping_list}) { > + my $entry = eval { parse_property_string($map_fmt, $map) }; > + warn $@ if $@; > + if ($entry && $entry->{node} eq $nodename) { > + push $res->@*, $entry; > + } > + } > + return $res; > +} > + > +PVE::Mapping::Dir->register(); > +PVE::Mapping::Dir->init(); > + > +1; _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel