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 90D1B91E8D for ; Wed, 31 Jan 2024 14:42:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 708CE3B401 for ; Wed, 31 Jan 2024 14:42:04 +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 14:42:03 +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 2FE8749384 for ; Wed, 31 Jan 2024 14:42:03 +0100 (CET) Message-ID: Date: Wed, 31 Jan 2024 14:42:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Fiona Ebner , Proxmox VE development discussion References: <20231108085254.53574-1-m.frank@proxmox.com> <20231108085254.53574-3-m.frank@proxmox.com> <38d62435-50ed-48ac-96f5-932e2be9e2c7@proxmox.com> From: Markus Frank In-Reply-To: <38d62435-50ed-48ac-96f5-932e2be9e2c7@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.032 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 13:42:04 -0000 Thanks for the review, 2 answers inline. The rest is clear. On 2024-01-31 13:01, Fiona Ebner wrote: > 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? I do not know why I wrote it like that. This is more correct: description => "Announce that the directory contains other mounted file systems.", > >> + 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? ACLs and xattrs just get ignored if not supported. > >> + }, >> + }, >> +}; >> + >> +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;