Re: [PATCH qemu-server 6/8] qemu: Add helpers for new custom models endpoints

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

Nit: the qemu-server patches should come first, since the UI patches
depend on them. And it's always good to mention in the cover letter
which inter-package dependencies there are, i.e. pve-manager needs a
versioned dependency bump for qemu-server. It's quite obvious in this
case, but still helps and mostly writing this for the future.

The prefix 'qemu' doesn't really add any information here. It should
rather be something like 'cpu config:'

Am 12.03.26 um 9:39 AM schrieb Arthur Bied-Charreton:
> Add functionality to lock & write the custom CPU config and some other
> helpers that will be needed in custom CPU models CRUD endpoints.
> 
> Original patch:
> https://lore.proxmox.com/pve-devel/20211028114150.3245864-5-s.reiter@proxmox.com/
> 
> Originally-by: Stefan Reiter <s.reiter@proxmox.com>

If you want, you can put a short high-level changelog here to document
how the patch changed, e.g.:

[AB: split patch into adding helpers and adding API endpoints
     some other change]

This can help follow history better later on.

> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  src/PVE/QemuServer/CPUConfig.pm | 35 ++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PVE/QemuServer/CPUConfig.pm b/src/PVE/QemuServer/CPUConfig.pm
> index 32ec4954..2b6665a7 100644
> --- a/src/PVE/QemuServer/CPUConfig.pm
> +++ b/src/PVE/QemuServer/CPUConfig.pm
> @@ -6,10 +6,10 @@ use warnings;
>  use JSON;
>  
>  use PVE::JSONSchema qw(json_bool);
> -use PVE::Cluster qw(cfs_register_file cfs_read_file);
> +use PVE::Cluster qw(cfs_register_file cfs_read_file cfs_write_file cfs_lock_file);
>  use PVE::ProcFSTools;
>  use PVE::RESTEnvironment qw(log_warn);
> -use PVE::Tools qw(run_command);
> +use PVE::Tools qw(run_command lock_file);
>  
>  use PVE::QemuServer::Helpers qw(min_version);
>  
> @@ -51,6 +51,19 @@ sub load_custom_model_conf {
>      return cfs_read_file($default_filename);
>  }
>  
> +sub write_custom_model_conf {
> +    my ($conf) = @_;
> +    cfs_write_file($default_filename, $conf);

Pre-existing, but could you add a preparatory commit renaming the
variable $default_filename to something more telling? Maybe
$cpu_models_filename?

> +}
> +
> +sub lock_cpu_config {
> +    my ($code) = @_;
> +    cfs_lock_file($default_filename, undef, $code);
> +    if (my $err = $@) {
> +        die $err;
> +    }

Style nit: could also just be
die $@ if $@;

> +}
> +
>  #builtin models : reported-model is mandatory
>  my $builtin_models_by_arch = {
>      x86_64 => {
> @@ -298,6 +311,19 @@ sub get_supported_cpu_flags {
>      return $supported_cpu_flags_by_arch->{$arch};
>  }
>  
> +sub description_by_flag {
> +    my ($arch) = @_;
> +    return { map { $_->{name} => $_->{description} } get_supported_cpu_flags($arch)->@* };
> +}
> +
> +sub get_cpu_vendor {
> +    my ($cputype, $arch) = @_;
> +    $arch = $host_arch if !defined($arch);

The $host_arch variable was recently dropped, needs a rebase.

> +    my $retval = $cpu_models_by_arch->{$arch}->{$cputype}
> +        or die "Built-in CPU type '$cputype' does not exist";
> +    return $retval;
> +}

These are quite small helpers and I do not see a big reason why they
should be added in a separate commit rather than just directly in the
commits with their users. It can be okay to add helpers up-front,
depending on the scenario, but here:
* 2 of the helpers are for config file handling
* 2 of the helpers are for cpu model property details, but also quite
unrelated
* the helpers are not complex

So I don't see enough semantic grouping that would justify having this
as a separate patch.

> +
>  my $all_supported_cpu_flags = {};
>  for my $arch ($supported_cpu_flags_by_arch->%*) {
>      for my $flag ($supported_cpu_flags_by_arch->{$arch}->@*) {
> @@ -375,6 +401,7 @@ my $cpu_fmt = {
>          optional => 1,
>      },
>  };
> +PVE::JSONSchema::register_standard_option('pve-qemu-cpu', $cpu_fmt);

I'd rather have this as a separate commit or squashed into the commit
with its first user.

>  
>  my $sev_fmt = {
>      type => {
> @@ -564,6 +591,7 @@ sub write_config {
>  
>          # saved in section header
>          delete $model_conf->{cputype};
> +        $model_conf->{type} = $class->type();

The commit message should at least describe why this is necessary. I
suppose for writing to actually work? Could also be a separate fix then,
but its fine to add together with the helper.

>      }
>  
>      $class->SUPER::write_config($filename, $cfg);
> @@ -612,7 +640,8 @@ sub get_cpu_models {
>  
>      my $conf = load_custom_model_conf();
>      for my $custom_model (keys %{ $conf->{ids} }) {
> -        my $reported_model = $conf->{ids}->{$custom_model}->{'reported-model'};
> +        my $model = $conf->{ids}->{$custom_model};
> +        my $reported_model = $model->{'reported-model'};

This change is unrelated to the rest.

>          $reported_model //= $cpu_fmt->{'reported-model'}->{default};
>          my $vendor = $all_cpu_models->{$reported_model};
>          push @$models,