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

2024-06-12 Thread Fiona Ebner
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 000..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



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

2024-05-14 Thread Markus Frank
Adds a config file for directories by using a 'map' property string for
each node mapping.

Next to node & path, there is the optional submounts parameter in the
map property string that is used to announce other mounted file systems
in the specified directory.

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 | 205 +
 2 files changed, 206 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 000..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;
+
+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';
+}
+
+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,
+},
+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,
+   },
+   },
+   xattr => {
+   type => 'boolean',
+   description => "Enable support for extended attributes."
+   ." If not supported by Guest OS or file system, this option is"
+   ." simply ignored.",
+   optional => 1,
+   default => 0,
+   },
+   acl => {
+   type => 'boolean',
+   description => "Enable support for POSIX ACLs (implies --xattr)."
+   ." The guest OS has to support ACLs. When used in a directory"
+   ." with a file system without ACL support, the ACLs are 
ignored.",
+   optional => 1,
+   default => 0,
+   },
+},
+};
+
+sub private {
+return $defaultData;
+}
+
+sub map_fmt {
+return $map_fmt;
+}
+
+sub options {
+return {
+   description => { optional => 1 },
+   map => {},
+   xattr => { optional => 1 },
+   acl => { optional => 1 },
+};
+}
+
+sub assert_valid {
+my ($dir_cfg) = @_;
+
+my $path = $dir_cfg->{path};
+
+if (! -e