Re: [pve-devel] [PATCH guest-common v10 2/11] add dir mapping section config

[Fiona Ebner <f.ebner@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