Re: [PATCH pve-manager v4 10/17] api: add CRUD handlers for custom CPU models

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

Am 30.04.26 um 6:02 PM schrieb Arthur Bied-Charreton:
> diff --git a/PVE/API2/Cluster/Qemu/CustomCPUModels.pm b/PVE/API2/Cluster/Qemu/CustomCPUModels.pm
> new file mode 100644
> index 00000000..8c2bd724
> --- /dev/null
> +++ b/PVE/API2/Cluster/Qemu/CustomCPUModels.pm
> @@ -0,0 +1,211 @@
> +package PVE::API2::Cluster::Qemu::CustomCPUModels;
> +
> +use v5.36;
> +
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RPCEnvironment;
> +use PVE::RESTHandler;
> +use PVE::Tools qw(extract_param);
> +
> +use PVE::QemuServer::CPUConfig;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    name => 'config',

The endpoints for mappings have a child link for 'id'. We could add one
for 'cputype'.

> +    path => '',
> +    method => 'GET',
> +    description => 'Read all custom CPU models definitions',
> +    permissions => {
> +        check => ['perm', '/nodes', ['Sys.Audit']],
> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +    },
> +    returns => {
> +        type => 'array',
> +        items => {
> +            type => 'object',
> +            properties => get_standard_option('pve-qemu-cpu'),
> +        },
> +    },
> +    code => sub {
> +        my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
> +        delete $conf->{order};
> +        my $ids = [];

Call it $res? It's not (just) the IDs.

> +        foreach my $id (keys %{ $conf->{ids} }) {

Style nits: prefer 'for' and '->%*'

> +            delete $conf->{ids}->{$id}->{type};
> +            push @$ids, $conf->{ids}->{$id};
> +        }
> +        return $ids;
> +    },
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'create',
> +    path => '',
> +    method => 'POST',
> +    protected => 1,
> +    description => 'Add a custom CPU model definition',
> +    permissions => {
> +        check => ['perm', '/nodes', ['Sys.Console']],

Should be Sys.Modify I guess?

> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
> +            digest => get_standard_option('pve-config-digest'),
> +        }),
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        my $digest = extract_param($param, 'digest');
> +
> +        (my $name = $param->{cputype}) =~ s/^custom-//;
> +        $param->{cputype} = "custom-$name";
> +

As mentioned in the qemu-server patch already, there could/should be a
check_config() call here.

> +        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
> +            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
> +            PVE::SectionConfig::assert_if_modified($conf, $digest);

Do we care? If the ID is still available, we can still proceed even if
there was another modification. We hold the lock.

> +
> +            die "custom CPU model '$name' already exists\n"
> +                if defined($conf->{ids}->{$name});
> +            $conf->{ids}->{$name} = $param;
> +
> +            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
> +        });
> +    },
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'delete',
> +    path => '{cputype}',
> +    method => 'DELETE',
> +    protected => 1,
> +    description => 'Delete a custom CPU model definition',
> +    permissions => {
> +        check => ['perm', '/nodes', ['Sys.Console']],

Sys.Modify?

> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {
> +            digest => get_standard_option('pve-config-digest'),
> +            cputype => {
> +                type => 'string',
> +                description => "The custom model to delete.",
> +            },
> +        },
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        my $digest = extract_param($param, 'digest');
> +
> +        (my $name = $param->{cputype}) =~ s/^custom-//;
> +
> +        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
> +            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
> +            PVE::SectionConfig::assert_if_modified($conf, $digest);

Similar here. If the ID still exists, we can delete it without worrying
about somebody having modified the config. I mean, you might want to
check if the ID does not exist to give a better error (since there might
have been a concurrent delete for the same ID).

> +
> +            die "custom CPU model '$name' does not exist\n"
> +                if !defined($conf->{ids}->{$name});
> +            delete $conf->{ids}->{$name};
> +
> +            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
> +        });
> +    },
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'update',
> +    path => '{cputype}',
> +    method => 'PUT',
> +    protected => 1,
> +    description => "Update a custom CPU model definition.",
> +    permissions => {
> +        check => ['perm', '/nodes', ['Sys.Console']],

Sys.Modify?

> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
> +            digest => get_standard_option('pve-config-digest'),
> +            delete => {
> +                type => 'string',
> +                format => 'pve-configid-list',
> +                description => "A list of properties to delete.",
> +                optional => 1,
> +            },
> +        }),
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        my $digest = extract_param($param, 'digest');
> +        my $delete = extract_param($param, 'delete') // '';
> +        my %delete_hash = map { $_ => 1 } PVE::Tools::split_list($delete);
> +
> +        (my $name = $param->{cputype}) =~ s/^custom-//;
> +        $param->{cputype} = "custom-$name";
> +
> +        PVE::QemuServer::CPUConfig::lock_cpu_config(sub {
> +            my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
> +
> +            PVE::SectionConfig::assert_if_modified($conf, $digest);

Of course, here it is very useful to abort :)

> +
> +            my $model = $conf->{ids}->{$name};
> +            die "custom CPU model '$name' does not exist\n" if !defined($model);
> +

As mentioned in the qemu-server patch already, there could/should be a
check_config() call here.

> +            my $props = PVE::QemuServer::CPUConfig::add_cpu_json_properties({});
> +            for my $p (keys %$props) {
> +                if ($delete_hash{$p}) {
> +                    die "cannot delete 'cputype' property\n" if $p eq 'cputype';
> +                    die "cannot set and delete property '$p' at once\n" if $param->{$p};
> +                    delete $model->{$p};

Please use the PVE::SectionConfig::delete_from_config() function.

> +                } elsif (defined($param->{$p})) {
> +                    $model->{$p} = $param->{$p};
> +                }
> +            }
> +
> +            PVE::QemuServer::CPUConfig::write_custom_model_conf($conf);
> +        });
> +    },
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'info',
> +    path => '{cputype}',
> +    method => 'GET',
> +    description => 'Retrieve details about a specific custom CPU model',
> +    permissions => {
> +        check => ['perm', '/nodes', ['Sys.Audit']],
> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {
> +            cputype => {
> +                type => 'string',
> +                description => "Name of the CPU model to query.",
> +            },
> +        },
> +    },
> +    returns => {
> +        type => 'object',
> +        properties => PVE::QemuServer::CPUConfig::add_cpu_json_properties({
> +            digest => get_standard_option('pve-config-digest'),
> +        }),
> +    },
> +    code => sub {
> +        my ($param) = @_;
> +        (my $name = $param->{cputype}) =~ s/^custom-//;
> +        my $conf = PVE::QemuServer::CPUConfig::load_custom_model_conf();
> +        my $digest = $conf->{digest};

Style nit: no need for an extra variable for the digest.

> +        my $retval = PVE::QemuServer::CPUConfig::get_custom_model($name);
> +        $retval->{digest} = $digest;
> +        return $retval;
> +    },
> +});
> +
> +1;
> diff --git a/PVE/API2/Cluster/Qemu/Makefile b/PVE/API2/Cluster/Qemu/Makefile
> index 2ab3e0b1..76b661db 100644
> --- a/PVE/API2/Cluster/Qemu/Makefile
> +++ b/PVE/API2/Cluster/Qemu/Makefile
> @@ -2,8 +2,9 @@ include ../../../../defines.mk
>  
>  # for node independent, cluster-wide applicable, API endpoints
>  # ensure we do not conflict with files shipped by pve-cluster!!
> -PERLSOURCE= 	\
> -	CPUFlags.pm
> +PERLSOURCE= 		\
> +	CPUFlags.pm		\
> +	CustomCPUModels.pm
>  
>  all:
>