Re: [pve-devel] [PATCH guest-common v2 1/1] add profiles section config plugin

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 14/11/2023 um 11:35 schrieb Dominik Csapak:
> this is intended to house custom profiles which can be used
> on guest creation instead of manually needing to specify every option.
> 
> we do special things here:
> * we always set 'allow_unknown' to 1, because when using the guest
>   specific parts in the cli, we cannot depend on the other one, else
>   we'd get a cyclic dependency, and without that we need to ignore
>   unknown sections
> 
> * we pack the type and id in the global options, so that when using
>   it with a seperated section config they get included in the
>   create/updateSchema
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/Makefile               |  2 ++
>  src/PVE/Profiles/Plugin.pm | 74 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
>  create mode 100644 src/PVE/Profiles/Plugin.pm
> 
> diff --git a/src/Makefile b/src/Makefile
> index cbc40c1..d99645c 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -17,6 +17,8 @@ install: PVE
>  	install -d ${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/Profiles
> +	install -m 0644 PVE/Profiles/Plugin.pm ${PERL5DIR}/PVE/Profiles/
>  	install -d ${PERL5DIR}/PVE/VZDump
>  	install -m 0644 PVE/VZDump/Plugin.pm ${PERL5DIR}/PVE/VZDump/
>  	install -m 0644 PVE/VZDump/Common.pm ${PERL5DIR}/PVE/VZDump/
> diff --git a/src/PVE/Profiles/Plugin.pm b/src/PVE/Profiles/Plugin.pm
> new file mode 100644
> index 0000000..6ea2b5f
> --- /dev/null
> +++ b/src/PVE/Profiles/Plugin.pm
> @@ -0,0 +1,74 @@
> +package PVE::Profiles::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Cluster qw(cfs_register_file);
> +use PVE::SectionConfig;
> +
> +use base qw(PVE::SectionConfig);
> +
> +my $CFG_PATH = 'virtual-guest/profiles.cfg';
> +
> +cfs_register_file ($CFG_PATH,

extra space before opening parenthesis.

> +		   sub { __PACKAGE__->parse_config(@_); },
> +		   sub { __PACKAGE__->write_config(@_); });

let's use some more modern indentation here

cfs_register_file(
    $CFG_PATH,
    sub { __PACKAGE__->parse_config(@_); },
    sub { __PACKAGE__->write_config(@_); },
);

> +
> +my $defaultData = {
> +    propertyList => {
> +	type => { description => 'Profile type.' },
> +	id => {
> +	    type => 'string',
> +	    description => "The ID of the profile.",
> +	    format => 'pve-configid',
> +	},
> +	'profile-description' => {
> +	    description => "Description.",

I mean, yes, but a sentence is still nice:

description => "Use this to add a short comment about a profile.",

> +	    type => 'string',
> +	    optional => 1,
> +	    maxLength => 4096,

would start out with a bit less, as this is rendered in grids and the like
and doesn't needs to hold that many information than e.g., node or guest
instances might have.

> +	},
> +    },
> +    options => {
> +	type => {},
> +	id => {},
> +	'profile-description' => { optional => 1 },
> +    },
> +};
> +
> +sub private {
> +    return $defaultData;
> +}
> +
> +sub parse_config {
> +    my ($class, $filename, $raw, $allow_unknown) = @_;
> +
> +    # always allow unknown, so that qemu-server/pct-container

to make it slightly easier to understand it'd IMO help to:
s/unknown/unknown section types/

> +    # can parse the file without loading the other plugin type
> +    return $class->SUPER::parse_config($filename, $raw, 1);
> +}
> +
> +sub write_config {
> +    my ($class, $filename, $cfg, $allow_unknown) = @_;
> +
> +    return $class->SUPER::write_config($filename, $cfg, 1);
> +}
> +
> +# gets, checks and prepares the guest config
> +# throws an error if it does not exist or the type is wrong
> +sub get_guest_ready_config {

This is rather covoluted name for what happens, wouldn't naming this
`load_profile` be clear enough? Or are there future changes, or overrides
in child-implementations that warrant this name (even then we probably
find something better)

> +    my ($id, $type) = @_;

Otherwise I'd use at least the following variable names to clarify what
$id's we're talking about, because from the method name I initially thought
those are vmid's (was cleared up quickly when continue reading the code, but
IMO still not ideal as is)

my ($profile_id, $profile_type) = @_;


> +
> +    my $cfg = PVE::Cluster::cfs_read_file($CFG_PATH);
> +
> +    my $profile = $cfg->{ids}->{$id};
> +    die "no such profile '$id'\n" if !defined $profile;
> +    die "wrong type '$profile->{type}'\n" if $profile->{type} ne $type;
> +
> +    delete $profile->{type};
> +    delete $profile->{'profile-description'};
> +
> +    return $profile;
> +}
> +
> +1;