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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 22.01.25 um 11:08 schrieb 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 announce-submounts parameter
> which forces virtiofsd to report a different device number for each
> submount it encounters. Without it, duplicates may be created because
> inode IDs are only unique on a single filesystem.
> 
> example config:
> ```
> some-dir-id
> 	map node=node1,path=/mnt/share/,announce-submounts=1
> 	map node=node2,path=/mnt/share/,
> ```
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>

Just a few small comments, if there will be a v14, those should be
addressed. Otherwise:

Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>

> +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',
> +    },
> +    'announce-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.",

Style nit: wrong indentation for the two lines above

> +	optional => 1,
> +	default => 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",

Nit: I'd clarify slightly more with "of the directory mapping"

> +	    format => 'pve-configid',
> +	},
> +	description => {
> +	    type => 'string',
> +	    description => "Description of the directory",

Nit: I'd clarify slightly more with "of the directory mapping"

> +	    optional => 1,
> +	    maxLength => 4096,
> +	},
> +	map => {
> +	    type => 'array',
> +	    description => 'A list of maps for the cluster nodes.',
> +	    optional => 1,
> +	    items => {
> +		type => 'string',
> +		format => $map_fmt,
> +	    },
> +	},
> +    },
> +};
> +
> +sub private {
> +    return $defaultData;
> +}
> +
> +sub map_fmt {
> +    return $map_fmt;
> +}

The map_fmt() subroutine is never called from anywhere or am I missing
something?

> +sub write_dir_config {
> +    my ($cfg) = @_;
> +
> +    cfs_write_file($FILENAME, $cfg);

Rather orthogonal to the series, but since this is a new configuration
file, should we start out with it always being UTF-8? I sent an RFC for
allowing to register such configuration files:

https://lore.proxmox.com/pve-devel/20250218123006.61691-1-f.ebner@proxmox.com/T/#t

If some proposal for registering UTF-8 configs is accepted before this
series lands, it would be nice to opt into it. But it's not a blocker
and could still be done when applying if everything else is ready.

> +}
> +
> +sub find_on_current_node {
> +    my ($id) = @_;
> +
> +    my $cfg = config();
> +    my $node = PVE::INotify::nodename();
> +
> +    return get_node_mapping($cfg, $id, $node);
> +}


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