From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3332A91CF1 for ; Wed, 31 Jan 2024 13:02:14 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0D6673A145 for ; Wed, 31 Jan 2024 13:01:44 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 31 Jan 2024 13:01:41 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 2EC564936F for ; Wed, 31 Jan 2024 13:01:41 +0100 (CET) Message-ID: <38d62435-50ed-48ac-96f5-932e2be9e2c7@proxmox.com> Date: Wed, 31 Jan 2024 13:01:35 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Proxmox VE development discussion , Markus Frank References: <20231108085254.53574-1-m.frank@proxmox.com> <20231108085254.53574-3-m.frank@proxmox.com> From: Fiona Ebner In-Reply-To: <20231108085254.53574-3-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.073 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 T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [dir.pm, plugin.pm] Subject: Re: [pve-devel] [PATCH guest-common v8 2/7] add Dir mapping config X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Jan 2024 12:02:14 -0000 Am 08.11.23 um 09:52 schrieb Markus Frank: > Adds a config file for directories by using a 'map' > array propertystring for each node mapping. > > Next to node & path, there is the optional > submounts parameter in the map array. > Additionally there are the default settings for xattr & acl. > > example config: > ``` > some-dir-id > map node=node1,path=/mnt/share/,submounts=1 > map node=node2,path=/mnt/share/, > xattr 1 > acl 1 > ``` > > Signed-off-by: Markus Frank > --- > src/Makefile | 1 + > src/PVE/Mapping/Dir.pm | 177 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 178 insertions(+) > create mode 100644 src/PVE/Mapping/Dir.pm > > diff --git a/src/PVE/Mapping/Dir.pm b/src/PVE/Mapping/Dir.pm > new file mode 100644 > index 0000000..6c87073 > --- /dev/null > +++ b/src/PVE/Mapping/Dir.pm > @@ -0,0 +1,177 @@ > +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::JSONSchema qw(get_standard_option parse_property_string); > +use PVE::SectionConfig; > +use PVE::INotify; Nit: not sorted alphabetically ---snip--- > +my $map_fmt = { > + node => get_standard_option('pve-node'), > + path => { > + description => "Directory-path that should be shared with the guest.", Nit: space instead of - is nicer IMHO and since it's an absolute path "Absolute directory path" > + type => 'string', > + format => 'pve-storage-path', This is registered in PVE/Storage/Plugin.pm To be clean, we should either add an include here (guest-common already depends on libpve-storage) or move registering the format to PVE::JSONSchema > + }, > + submounts => { > + type => 'boolean', > + description => "Option to tell the guest which directories are mount points.", I haven't looked at the code where this is used yet, so I'm as confused as a user now ;) Does this affect mount points within the directory (which the "sub" prefix suggests)? But it's a boolean, so how can I tell "which directories"? The description should answer: When does it need to be set/what effect does it have? > + optional => 1, > + }, > + description => { > + description => "Description of the node specific directory.", > + type => 'string', > + optional => 1, > + maxLength => 4096, > + }, > +}; > + > +my $defaultData = { > + propertyList => { > + id => { > + type => 'string', > + description => "The ID of the directory", > + format => 'pve-configid', > + }, > + description => { > + description => "Description of the directory", > + type => 'string', > + optional => 1, > + maxLength => 4096, > + }, > + map => { > + type => 'array', > + description => 'A list of maps for the cluster nodes.', > + optional => 1, > + items => { > + type => 'string', > + format => $map_fmt, > + }, > + }, Style nit: Indentation is mostly wrong since the beginning of $defaultData until here. > + xattr => { > + type => 'boolean', > + description => "Enable support for extended attributes.", > + optional => 1, > + }, > + acl => { > + type => 'boolean', > + description => "Enable support for posix ACLs (implies --xattr).", s/posix/POSIX/ > + optional => 1, What could also be mentioned for xattr and acl: do the underlying file systems need to support these? What happens if they don't? > + }, > + }, > +}; > + > +sub private { > + return $defaultData; > +} > + > +sub options { > + return { > + description => { optional => 1 }, > + map => {}, > + xattr => { optional => 1 }, > + acl => { optional => 1 }, Style nit: wrong indentation > + }; > +} > + > +sub assert_valid { > + my ($dir_cfg) = @_; > + > + my $path = $dir_cfg->{path}; > + > + if (! -e $path) { > + die "Path $path does not exist\n"; Style nit: wrong indentation > + } > + if ((-e $path) && (! -d $path)) { Style nit: could be made into an elsif to avoid an extra -e check > + die "Path $path exists but is not a directory\n" Style nit: wrong indentation > + } > + > + return 1; > +}; > + > +sub config { > + return cfs_read_file($FILENAME); > +} > + > +sub lock_dir_config { > + my ($code, $errmsg) = @_; > + > + cfs_lock_file($FILENAME, undef, $code); > + my $err = $@; > + if ($err) { Style nit: could be if (my $err = $@) { > + $errmsg ? die "$errmsg: $err" : die $err; Style nit: wrong indentation > + } > +} > + > +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(); > + > + return get_node_mapping($cfg, $id, $node); > +} > + > +sub get_node_mapping { > + my ($cfg, $id, $nodename) = @_; > + > + return undef if !defined($cfg->{ids}->{$id}); > + > + my $res = []; > + my $mapping_list = $cfg->{ids}->{$id}->{map}; > + foreach my $map (@{$mapping_list}) { Style nit: new code should use "for" > + 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;