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 [212.224.123.68])
	by lore.proxmox.com (Postfix) with ESMTPS id E6C9D1FF391
	for <inbox@lore.proxmox.com>; Wed, 12 Jun 2024 13:49:33 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
	by firstgate.proxmox.com (Proxmox) with ESMTP id 3B239F1B4;
	Wed, 12 Jun 2024 13:50:10 +0200 (CEST)
Message-ID: <0cbfe3d2-237d-4650-868f-7689b7e079e4@proxmox.com>
Date: Wed, 12 Jun 2024 13:50: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: <20240514105442.1187746-1-m.frank@proxmox.com>
 <20240514105442.1187746-3-m.frank@proxmox.com>
Content-Language: en-US
From: Fiona Ebner <f.ebner@proxmox.com>
In-Reply-To: <20240514105442.1187746-3-m.frank@proxmox.com>
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.059 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 -
Subject: Re: [pve-devel] [PATCH guest-common v10 2/11] 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-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>

Am 14.05.24 um 12:54 schrieb Markus Frank:
> diff --git a/src/PVE/Mapping/Dir.pm b/src/PVE/Mapping/Dir.pm
> new file mode 100644
> index 0000000..8f131c2
> --- /dev/null
> +++ b/src/PVE/Mapping/Dir.pm
> @@ -0,0 +1,205 @@
> +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 PVE::Storage::Plugin;

Storage::Plugin isn't used anywhere?

> +
> +use base qw(PVE::SectionConfig);
> +
> +my $FILENAME = 'mapping/dir.cfg';
> +
> +cfs_register_file($FILENAME,
> +                  sub { __PACKAGE__->parse_config(@_); },
> +                  sub { __PACKAGE__->write_config(@_); });

Style nit: uses only spaces, non-standard indentation

---snip---

> +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',
> +    },
> +    submounts => {
> +	type => 'boolean',
> +	description => "Announce that the directory contains other mounted"
> +	    ." file systems. If this is not set and multiple file systems are"
> +	    ." mounted, the guest may encounter duplicates due to file system"
> +	    ." specific inode IDs.",
> +	optional => 1,
> +	default => 0,

Hmm, so even if this option is not set, the files from submounts are
accessible? Should this rather be turned on by default or always? Or
what are the downsides when setting this?

---snip---

> +sub assert_valid {
> +    my ($dir_cfg) = @_;
> +
> +    my $path = $dir_cfg->{path};
> +
> +    if (! -e $path) {
> +	die "Path $path does not exist\n";
> +    } elsif (! -d $path) {
> +	die "Path $path exists but is not a directory\n"

Nit, missing comma before 'but', missing semicolon at the end of the line.

> +    }
> +
> +    return 1;
> +};
> +
> +sub check_duplicate {

Better called e.g. assert_no_duplicate_node, because it dies. "check" is
mostly used for functions returning a "boolean" in our code base.

> +    my ($map_list) = @_;
> +
> +    my %count;
> +    for my $map (@$map_list){

Style nit: missing space after closing parenthesis

> +	my $entry = parse_property_string($map_fmt, $map);
> +	$count{$entry->{node}}++;> +    }
> +    for my $node (keys %count) {
> +	if ($count{$node} > 1) {
> +	    die "Node '$node' is specified $count{$node} times.\n";
> +	}
> +    }
> +}
> +


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel